-
Notifications
You must be signed in to change notification settings - Fork 404
fix(firestore-send-email): preparePayload fix #2423
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors payload preparation for sending emails by extracting the preparePayload logic into its own module using dependency injection and enhancing its unit tests.
- Introduces a new preparePayload module with dependency injection support.
- Removes the inline preparePayload implementation from index.ts.
- Adds comprehensive unit tests covering various payload scenarios.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
firestore-send-email/functions/src/prepare-payload.ts | New module for payload preparation with dependency injection and template merging. |
firestore-send-email/functions/src/index.ts | Removal of inline preparePayload; added dependency injection initialization. |
firestore-send-email/functions/tests/validation.test.ts | Added a test case for payload validation when no message object is provided. |
firestore-send-email/functions/tests/prepare-payload.test.ts | New tests to cover multiple scenarios for template and message merging in payloads. |
firestore-send-email/CHANGELOG.md | Changelog update with a fix entry for payload validation. |
Comments suppressed due to low confidence (1)
firestore-send-email/functions/src/prepare-payload.ts:6
- Consider using more specific types instead of 'any' for 'db' to improve type safety and enable better IntelliSense.
let db: any;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the payload preparation logic into a dedicated module with dependency injection, enhances validation for attachments, and updates test and CI configurations to improve coverage.
- Extract
preparePayload
intoprepare-payload.ts
and wire it viasetDependencies
- Add Zod schemas for attachments and adjust message schema to allow nullable text/html
- Update tests to cover attachment validation and add coverage tooling
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
functions/src/validation.ts | Added attachmentSchema and attachmentsSchema , updated baseMessageSchema to allow nullable content |
functions/src/prepare-payload.ts | New module: DI for Firestore and template engine, unified payload validation and merging logic |
functions/src/index.ts | Removed inline preparePayload , now imports and configures it via setDependencies |
functions/package.json | Added coverage and SendGrid E2E test scripts |
functions/jest.config.js | Configured coverage collection and reporting |
functions/tests/validation.test.ts | Added unit tests for various attachment scenarios |
functions/tests/e2e/sendgrid.test.ts | New E2E tests for SendGrid integration |
CHANGELOG.md | Updated to version 0.2.2 with validation fix entry |
Comments suppressed due to low confidence (1)
firestore-send-email/functions/src/prepare-payload.ts:29
- The new
preparePayload
module encapsulates key logic (template merging and UID-to-email resolution) but lacks dedicated unit tests. Add tests for payload merging, attachment handling, and UID lookup behavior to ensure this logic stays correct.
export async function preparePayload(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm
This PR is to actually fix #2419
We extract preparePayload into a dependency injection pattern, and cover it with unit tests, and ensure template/message merging is being done correctly