Conversation
…om:CISCODE-MA/NotificationKit into develop
- Replace GitHub Actions version tags with full commit SHA hashes (v6 -> fd88b7d, v1 -> d304d05) - Fix SONAR_PROJECT_KEY from LoggingKit to NotificationKit - Add 'main' branch to PR trigger list for release-check workflow - Resolves 2 security hotspots (githubactions:S7637) for supply chain security
- Add explicit NotificationDocument type annotations to map callbacks - Fix mapToRecord signature to accept any type (handles both Map and Record) - Install mongoose as dev dependency for type checking - Fixes pre-push typecheck failures
- Add comprehensive instruction files in .github/instructions/ - Includes copilot, testing, bugfix, features, general guidelines - Standardize documentation across all repositories
- Remove deprecated instruction files from .github/ root - Consolidate all docs in .github/instructions/ directory - Improve documentation organization
- Mongoose required for type compilation (infra/repositories/mongoose) - ts-node required by Jest configuration - Resolves typecheck and test errors
* implemented decorators and providers * Add notification and webhook controller tests * remove in-memory repository and update exports * removed mongoose * removed duplicate code for sonarqube * docs: add comprehensive documentation for testing implementation * style: fix prettier formatting issues
|
There was a problem hiding this comment.
Pull request overview
This PR significantly expands @ciscode/notification-kit by adding a NestJS integration layer (module/controllers/DI tokens), infrastructure adapters (senders, providers, schema reference), and a large test/documentation/CI uplift to support multi-channel notifications as a reusable package.
Changes:
- Added NestJS module integration (register/registerAsync), controllers (REST + webhook), and DI helpers.
- Added infra layer exports: sender adapters (Email/SMS/Push), utility providers, and a Mongoose schema reference.
- Added extensive tests, docs, CI/publishing workflow updates, and a changeset.
Reviewed changes
Copilot reviewed 54 out of 56 changed files in this pull request and generated 18 comments.
Show a summary per file
| File | Description |
|---|---|
| tsup.config.ts | Marks Nest/provider SDKs as externals for library bundling. |
| tsconfig.json | Enables decorators/metadata and adds DOM lib/types for Node fetch usage. |
| test/test-utils.ts | Adds shared mocks/factories for unit/integration tests. |
| test/smoke.spec.ts | Replaces trivial smoke test with export-surface checks. |
| test/integration.test.ts | Adds end-to-end-ish module/service integration coverage. |
| src/nest/providers.ts | Adds provider factory for Nest DI tokens and NotificationService. |
| src/nest/module.ts | Implements NotificationKitModule.register() / registerAsync(). |
| src/nest/module.test.ts | Adds tests around module registration and DI wiring. |
| src/nest/interfaces.ts | Defines module options + async options factory interfaces. |
| src/nest/index.ts | Exports Nest layer public surface. |
| src/nest/decorators.ts | Adds convenience @Inject*() decorators for tokens. |
| src/nest/decorators.test.ts | Adds tests for decorators/tokens. |
| src/nest/controllers/webhook.controller.ts | Adds webhook receiver controller for delivery callbacks. |
| src/nest/controllers/webhook.controller.test.ts | Adds webhook controller unit tests. |
| src/nest/controllers/notification.controller.ts | Adds REST controller for send/query/retry/cancel operations. |
| src/nest/controllers/notification.controller.test.ts | Adds REST controller unit tests. |
| src/nest/constants.ts | Defines DI token symbols. |
| src/infra/senders/sms/vonage.sender.ts | Adds Vonage SMS sender adapter (lazy SDK import). |
| src/infra/senders/sms/twilio.sender.ts | Adds Twilio SMS sender adapter (lazy SDK import). |
| src/infra/senders/sms/aws-sns.sender.ts | Adds AWS SNS SMS sender adapter (lazy SDK import). |
| src/infra/senders/push/onesignal.sender.ts | Adds OneSignal push sender using fetch. |
| src/infra/senders/push/firebase.sender.ts | Adds Firebase push sender adapter (lazy SDK import). |
| src/infra/senders/push/aws-sns-push.sender.ts | Adds AWS SNS push sender adapter (lazy SDK import). |
| src/infra/senders/index.ts | Aggregates infra sender exports. |
| src/infra/senders/email/nodemailer.sender.ts | Adds Nodemailer email sender adapter (lazy import). |
| src/infra/repositories/mongoose/notification.schema.ts | Adds a Mongoose schema definition as a reference export. |
| src/infra/repositories/index.ts | Exposes repository schema reference exports. |
| src/infra/providers/template.provider.ts | Adds template engines (Handlebars + simple replacement). |
| src/infra/providers/providers.test.ts | Adds tests for provider utilities (id/datetime/templates/events). |
| src/infra/providers/index.ts | Aggregates provider exports. |
| src/infra/providers/id-generator.provider.ts | Adds UUID/ObjectId/NanoId generators. |
| src/infra/providers/event-emitter.provider.ts | Adds in-memory + console event emitters. |
| src/infra/providers/datetime.provider.ts | Adds datetime provider implementation. |
| src/infra/index.ts | Introduces infra layer barrel export. |
| src/infra/README.md | Expands infra documentation and configuration examples. |
| src/index.ts | Exports core + infra + nest from the root entry. |
| src/core/notification.service.test.ts | Adds broad unit coverage for core service behavior. |
| src/core/errors.test.ts | Adds tests for domain error types. |
| src/core/dtos.test.ts | Adds tests for Zod DTO schemas/helpers. |
| package.json | Adds optional peer meta, dev deps, and retains root-only export map. |
| jest.config.ts | Expands match patterns and adds coverage thresholds/reporters. |
| README.md | Adds comprehensive package docs and usage examples. |
| CONTRIBUTING.md | Updates contribution/testing guidance. |
| CHANGELOG.md | Adds a changelog scaffold and initial entries. |
| .github/workflows/release-check.yml | Updates CI branch targets and Sonar project key + pins actions. |
| .github/workflows/publish.yml | Changes publish trigger/flow and adds provenance publishing. |
| .github/instructions/testing.instructions.md | Adds detailed internal testing guidelines. |
| .github/instructions/sonarqube_mcp.instructions.md | Adds SonarQube MCP usage instructions. |
| .github/instructions/general.instructions.md | Adds general module development guidelines. |
| .github/instructions/features.instructions.md | Adds feature development playbook. |
| .github/instructions/bugfix.instructions.md | Adds bugfix process playbook. |
| .github/dependabot.yml | Adds Dependabot configuration for npm/actions. |
| .changeset/notificationkit_71368.md | Adds changeset describing the feature/test/doc uplift. |
| .github/copilot-instructions.md | Removes previous template copilot instructions file. |
| import { NotificationService } from "../core/notification.service"; | ||
| import { DateTimeProvider as _DateTimeProvider } from "../infra/providers/datetime.provider"; | ||
| import { UuidGenerator as _UuidGenerator } from "../infra/providers/id-generator.provider"; | ||
|
|
There was a problem hiding this comment.
Unused imports DateTimeProvider as _DateTimeProvider and UuidGenerator as _UuidGenerator add noise and may fail linting. Remove them or use them directly (instead of dynamic importing again) if they were intended to force inclusion.
| @Post("send") | ||
| @HttpCode(HttpStatus.CREATED) | ||
| async send(@Body() dto: SendNotificationDto) { | ||
| try { | ||
| return await this.notificationService.send(dto); | ||
| } catch (error) { | ||
| if (error instanceof ValidationError) { | ||
| throw new BadRequestException(error.message); | ||
| } | ||
| throw error; | ||
| } |
There was a problem hiding this comment.
These endpoints accept SendNotificationDto/CreateNotificationDto directly from @Body() but there is no runtime validation/parsing (Zod schemas exist in src/core/dtos.ts but NotificationService doesn't call them). As a result, invalid inputs may flow into the core service, and the ValidationError catch here is currently ineffective. Consider validating/parsing with the Zod schemas in the controller (or via a custom Nest pipe) and mapping Zod errors to BadRequestException.
| "peerDependenciesMeta": { | ||
| "nodemailer": { | ||
| "optional": true | ||
| }, | ||
| "twilio": { | ||
| "optional": true | ||
| }, | ||
| "@aws-sdk/client-sns": { | ||
| "optional": true | ||
| }, | ||
| "@vonage/server-sdk": { | ||
| "optional": true | ||
| }, | ||
| "firebase-admin": { | ||
| "optional": true | ||
| }, | ||
| "mongoose": { | ||
| "optional": true | ||
| }, | ||
| "handlebars": { | ||
| "optional": true | ||
| }, | ||
| "nanoid": { | ||
| "optional": true | ||
| } | ||
| }, |
There was a problem hiding this comment.
peerDependenciesMeta marks provider SDKs as optional, but they are not listed in peerDependencies. Many package managers ignore peerDependenciesMeta entries that don't correspond to a declared peer dependency, so consumers won't get correct install-time hints/warnings. Add these optional packages to peerDependencies (with appropriate version ranges) and keep them marked optional via peerDependenciesMeta.
| ```typescript | ||
| import { NodemailerSender } from "@ciscode/notification-kit/infra"; | ||
|
|
||
| const emailSender = new NodemailerSender({ | ||
| host: "smtp.gmail.com", |
There was a problem hiding this comment.
This document also uses subpath imports like @ciscode/notification-kit/infra, but the package currently exports only the root entry (exports has only "."). Align the docs with the actual package exports, or add subpath exports if you intend consumers to import infra directly.
| on: | ||
| push: | ||
| tags: | ||
| - "v*.*.*" | ||
| branches: | ||
| - master | ||
| workflow_dispatch: | ||
|
|
||
| jobs: | ||
| publish: | ||
| runs-on: ubuntu-latest | ||
|
|
||
| permissions: | ||
| contents: read | ||
| packages: write | ||
| id-token: write | ||
|
|
||
| steps: | ||
| - name: Checkout code | ||
| uses: actions/checkout@v4 | ||
| with: | ||
| fetch-depth: 0 | ||
|
|
||
| - name: Validate tag exists on this push | ||
| run: | | ||
| TAG=$(git describe --exact-match --tags HEAD 2>/dev/null || echo "") | ||
| if [[ -z "$TAG" ]]; then | ||
| echo "❌ No tag found on HEAD. This push did not include a version tag." | ||
| echo "To publish, merge to master with a tag: git tag v1.0.0 && git push origin master --tags" | ||
| exit 1 | ||
| fi | ||
| if [[ ! "$TAG" =~ ^v[0-9]+\.[0-9]+\.[0-9]+$ ]]; then | ||
| echo "❌ Invalid tag format: $TAG. Expected: v*.*.*" | ||
| exit 1 | ||
| fi | ||
| echo "✅ Valid tag found: $TAG" | ||
| echo "TAG_VERSION=$TAG" >> $GITHUB_ENV |
There was a problem hiding this comment.
publish.yml is triggered on every push to master, but then hard-fails if HEAD is not tagged. This will cause repeated failed workflow runs for normal merges. If publishing should only happen on tagged releases, consider restoring a on.push.tags: ["v*.*.*"] trigger (or use workflow_run from a dedicated release workflow) instead of failing after the fact.
| ```typescript | ||
| import { Module } from "@nestjs/common"; | ||
| import { NotificationKitModule } from "@ciscode/notification-kit"; | ||
| import { NodemailerSender, MongooseNotificationRepository } from "@ciscode/notification-kit/infra"; | ||
|
|
||
| @Module({ | ||
| imports: [ | ||
| NotificationKitModule.register({ | ||
| senders: [ | ||
| new NodemailerSender({ | ||
| host: "smtp.gmail.com", | ||
| port: 587, | ||
| secure: false, | ||
| auth: { | ||
| user: process.env.EMAIL_USER, | ||
| pass: process.env.EMAIL_PASSWORD, | ||
| }, | ||
| from: "noreply@example.com", | ||
| }), | ||
| ], | ||
| repository: new MongooseNotificationRepository(/* mongoose connection */), | ||
| }), |
There was a problem hiding this comment.
The README example imports MongooseNotificationRepository from @ciscode/notification-kit/infra, but this package only exports a Mongoose schema reference (concrete repositories are in separate packages). Update the example to use the external repository package (e.g. @ciscode/notification-kit-mongodb) or remove the repository class from the snippet to avoid misleading consumers.
| branches: 70, | ||
| functions: 70, | ||
| lines: 75, | ||
| statements: 75, |
There was a problem hiding this comment.
Jest coverageThreshold is set to 70–75%, but the repo docs/instructions in this PR state an 80%+ target. Align the configured thresholds with the documented standard (or update the docs if 70–75% is the intended target).
| branches: 70, | |
| functions: 70, | |
| lines: 75, | |
| statements: 75, | |
| branches: 80, | |
| functions: 80, | |
| lines: 80, | |
| statements: 80, |
| @Controller() | ||
| export class NotificationController { | ||
| private readonly prefix: string; | ||
|
|
||
| constructor( | ||
| @Inject(NOTIFICATION_SERVICE) | ||
| private readonly notificationService: NotificationService, | ||
| @Inject(NOTIFICATION_KIT_OPTIONS) | ||
| private readonly options: NotificationKitModuleOptions, | ||
| ) { | ||
| this.prefix = options.apiPrefix || "notifications"; | ||
| } |
There was a problem hiding this comment.
apiPrefix is assigned to this.prefix but never used by any route decorators, so consumers setting apiPrefix will see no effect. Either remove the option or implement prefixing at the controller level (e.g., fixed @Controller('notifications')) and document that dynamic prefixes are not supported by NestJS without generating controllers dynamically.
| } else { | ||
| results.push({ | ||
| success: false, | ||
| notificationId: item.notificationId, | ||
| error: String(error), | ||
| }); | ||
| } |
There was a problem hiding this comment.
handleWebhook() returns error: String(error) in the per-item results, which can leak internal error details (and potentially stack traces) to the webhook caller. Consider returning a stable, sanitized error code/message and logging the full error server-side instead.
| static registerAsync(options: NotificationKitModuleAsyncOptions): DynamicModule { | ||
| const asyncOptionsProvider = this.createAsyncOptionsProvider(options); | ||
| const asyncProviders = this.createAsyncProviders(options); | ||
|
|
||
| // We can't conditionally load controllers in async mode without the options | ||
| // So we'll need to always include them and they can handle being disabled internally | ||
| // Or we can create a factory provider that returns empty array | ||
| const providersFactory: Provider = { | ||
| provide: "NOTIFICATION_PROVIDERS", | ||
| useFactory: (moduleOptions: NotificationKitModuleOptions) => { | ||
| return createNotificationKitProviders(moduleOptions); | ||
| }, | ||
| inject: [NOTIFICATION_KIT_OPTIONS], | ||
| }; | ||
|
|
||
| const allProviders = [asyncOptionsProvider, ...asyncProviders, providersFactory]; | ||
| const exports = allProviders.map((p) => | ||
| typeof p === "object" && "provide" in p ? p.provide : p, | ||
| ); | ||
|
|
||
| return { | ||
| global: true, | ||
| module: NotificationKitModule, | ||
| imports: options.imports || [], | ||
| controllers: [], // Controllers disabled in async mode for simplicity | ||
| providers: allProviders, | ||
| exports, | ||
| }; |
There was a problem hiding this comment.
registerAsync() does not actually register the NotificationKit providers (senders/repository/id generator/datetime/service). The providersFactory returns an array of providers, but Nest will not auto-register providers returned from another provider, so NOTIFICATION_SERVICE (and other tokens from createNotificationKitProviders) will be missing in async mode. Refactor async registration to create the same providers as register() (e.g., providers whose useFactory reads from NOTIFICATION_KIT_OPTIONS, plus the NOTIFICATION_SERVICE factory), and ensure the exported tokens include NOTIFICATION_SERVICE and related dependencies.
|
@y-aithnini I've opened a new pull request, #7, to work on those changes. Once the pull request is ready, I'll request review from you. |
Zaiidmo
left a comment
Zaiidmo
left a comment
There was a problem hiding this comment.
Fix the merging cinflicts
Summary
Why
Checklist
npm run lintpassesnpm run typecheckpassesnpm testpassesnpm run buildpassesnpx changeset) if this affects consumersNotes