Refactor email service to pluggable facade architecture with mock for local development #296
+10,195
−1,168
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…reated event Co-authored-by: rohit-r-kumar <175348946+rohit-r-kumar@users.noreply.github.com>
Co-authored-by: rohit-r-kumar <175348946+rohit-r-kumar@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Refactor service-sendgrid package for local development mock
Refactor email service to pluggable facade architecture with mock for local development
Nov 20, 2025
packages/sthrift/persistence/COVERAGE_AND_DUPLICATION_RESOLUTION.md
Outdated
Show resolved
Hide resolved
packages/sthrift/persistence/src/test-utilities/conversation/conversation-test-helpers.ts
Outdated
Show resolved
Hide resolved
jason-t-hankins
previously approved these changes
Nov 21, 2025
Contributor
|
@sourcery-ai review |
Contributor
Reviewer's GuideIntroduces a provider-agnostic transactional email facade with SendGrid and filesystem-backed mock implementations, wires the email service into the API and event-handling stack to notify listing owners when reservation requests are created, and refactors persistence tests/utilities (especially reservation request and conversation read repositories) to use dedicated data sources, converters, and shared mock helpers. Sequence diagram for reservation request email notification via transactional email servicesequenceDiagram
actor ReserverUser
participant UI as UiFrontend
participant Api as ApiFunction
participant AppServices as ApplicationServices
participant Domain as ReservationRequestAggregate
participant EventBus as EventBusInstance
participant Handler as ReservationRequestCreatedHandler
participant NotifySvc as ReservationRequestNotificationService
participant DomainDS as DomainDataSource
participant EmailSvc as TransactionalEmailService
ReserverUser->>UI: Create_reservation_request
UI->>Api: HTTP_request_createReservationRequest
Api->>AppServices: createReservationRequestCommand
AppServices->>Domain: ReservationRequest_create
Domain->>Domain: set_state_requested_and_add_integration_event
Domain->>EventBus: publish ReservationRequestCreated
EventBus-->>Handler: invoke_with_ReservationRequestCreated_payload
Handler->>NotifySvc: sendReservationRequestNotification(reservationRequestId,listingId,reserverId,sharerId,reservationPeriodStart,reservationPeriodEnd)
Note over NotifySvc,DomainDS: Notification service uses UnitOfWork API on DomainDataSource
NotifySvc->>DomainDS: PersonalUserUnitOfWork.withTransaction_get_sharer
NotifySvc->>DomainDS: PersonalUserUnitOfWork.withTransaction_get_reserver
NotifySvc->>DomainDS: ItemListingUnitOfWork.withTransaction_get_listing
alt valid_email_and_entities
NotifySvc->>EmailSvc: sendTemplatedEmail(templateName,recipient,templateData)
EmailSvc-->>NotifySvc: Promise_resolved
else missing_data_or_error
NotifySvc-->>Handler: log_error_and_return
end
Handler-->>EventBus: completion
EventBus-->>AppServices: return
AppServices-->>Api: result
Api-->>UI: HTTP_response_success
UI-->>ReserverUser: success_message
Class diagram for transactional email facade and reservation request notification flowclassDiagram
class TransactionalEmailService {
<<interface>>
+startUp() Promise~void~
+shutDown() Promise~void~
+sendTemplatedEmail(templateName string, recipient EmailRecipient, templateData EmailTemplateData) Promise~void~
}
class EmailRecipient {
+email string
+name string
}
class EmailTemplateData {
}
class TemplateUtils {
-baseTemplateDir string
+TemplateUtils()
+loadTemplate(templateName string) EmailTemplate
+substituteVariables(template string, data EmailTemplateData) string
}
class EmailTemplate {
+fromEmail string
+subject string
+body string
}
class ServiceTransactionalEmailSendGrid {
-templateUtils TemplateUtils
+ServiceTransactionalEmailSendGrid()
+startUp() Promise~void~
+shutDown() Promise~void~
+sendTemplatedEmail(templateName string, recipient EmailRecipient, templateData EmailTemplateData) Promise~void~
}
class ServiceTransactionalEmailMock {
-outputDir string
-templateUtils TemplateUtils
+ServiceTransactionalEmailMock()
+startUp() Promise~void~
+shutDown() Promise~void~
+sendTemplatedEmail(templateName string, recipient EmailRecipient, templateData EmailTemplateData) Promise~void~
-createEmailHtml(recipient EmailRecipient, subject string, from string, bodyHtml string) string
-escapeHtml(text string) string
}
class ReservationRequestCreatedProps {
+reservationRequestId string
+listingId string
+reserverId string
+sharerId string
+reservationPeriodStart Date
+reservationPeriodEnd Date
}
class ReservationRequestCreated {
}
class ReservationRequestNotificationService {
-domainDataSource any
-emailService TransactionalEmailService
+ReservationRequestNotificationService(domainDataSource any, emailService TransactionalEmailService)
+sendReservationRequestNotification(reservationRequestId string, listingId string, reserverId string, sharerId string, reservationPeriodStart Date, reservationPeriodEnd Date) Promise~void~
}
class ReservationRequest {
-isNew boolean
-visa ReservationRequestVisa
-_listingId string
-_reserverId string
-_sharerId string
+create(props ReservationRequestProps, passport Passport) ReservationRequest
+request() void
+addIntegrationEvent(eventType any, payload any) void
}
class DomainDataSource {
}
TransactionalEmailService <|.. ServiceTransactionalEmailSendGrid
TransactionalEmailService <|.. ServiceTransactionalEmailMock
TemplateUtils --> EmailTemplate
ServiceTransactionalEmailSendGrid --> TemplateUtils
ServiceTransactionalEmailMock --> TemplateUtils
ReservationRequestCreated --* ReservationRequestCreatedProps
ReservationRequestNotificationService --> TransactionalEmailService
ReservationRequestNotificationService --> DomainDataSource
ReservationRequest ..> ReservationRequestCreated : emits
ReservationRequest ..> ReservationRequestCreatedProps
EmailRecipient ..> EmailTemplateData
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The new ReservationRequestCreated event type is not exported from the @sthrift/domain barrel (events/index.ts and/or domain/index.ts), but the handler imports it as
ReservationRequestCreatedfrom '@sthrift/domain', which will fail unless you add the appropriate re-exports. - ServiceTransactionalEmailSendGrid throws immediately if SENDGRID_API_KEY is missing, but the API currently selects the implementation based only on NODE_ENV; consider choosing the provider based on the presence of the API key (or deferring SendGrid instantiation) so non-dev environments without a key don’t crash at startup.
- Both the mock and SendGrid transactional email services duplicate template loading and variable substitution logic; consider extracting this into a shared utility to keep behavior consistent and reduce maintenance overhead.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new ReservationRequestCreated event type is not exported from the @sthrift/domain barrel (events/index.ts and/or domain/index.ts), but the handler imports it as `ReservationRequestCreated` from '@sthrift/domain', which will fail unless you add the appropriate re-exports.
- ServiceTransactionalEmailSendGrid throws immediately if SENDGRID_API_KEY is missing, but the API currently selects the implementation based only on NODE_ENV; consider choosing the provider based on the presence of the API key (or deferring SendGrid instantiation) so non-dev environments without a key don’t crash at startup.
- Both the mock and SendGrid transactional email services duplicate template loading and variable substitution logic; consider extracting this into a shared utility to keep behavior consistent and reduce maintenance overhead.
## Individual Comments
### Comment 1
<location> `packages/sthrift/persistence/src/datasources/readonly/reservation-request/reservation-request/reservation-request.read-repository.test.ts:106` </location>
<code_context>
+ } as unknown as Domain.Contexts.ReservationRequest.ReservationRequest.ReservationRequestEntityReference;
+}
+
+test.for(feature, ({ Scenario, BeforeEachScenario, Background }) => {
+ let models: ModelsContext;
+ let passport: Domain.Passport;
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring these tests to introduce shared helpers, focus more on observable behavior than internal calls, and centralize mock setup to reduce duplication and verbosity.
You can keep the new data‑source/converter integration but still reduce complexity and duplication.
Key issues:
- Many scenarios re‑encode the same filters and `ObjectId` constructions.
- Most scenarios assert internal calls instead of returned behavior.
- Constructor mocking for `ReservationRequestDataSourceImpl` and `ReservationRequestConverter` adds boilerplate.
Concrete suggestions:
1) Extract reusable helpers for filters and ObjectIds
Right now you have lots of repeated:
```ts
new MongooseSeedwork.ObjectId(TEST_IDS.USER_1)
new MongooseSeedwork.ObjectId(TEST_IDS.LISTING_1)
```
and inline filters like:
```ts
{ reserver: new MongooseSeedwork.ObjectId(TEST_IDS.USER_1) }
```
You can centralize those into helpers to both:
- reduce repetition, and
- keep the scenarios more declarative.
Example:
```ts
const TEST_OBJECT_IDS = {
USER_1: new MongooseSeedwork.ObjectId(TEST_IDS.USER_1),
LISTING_1: new MongooseSeedwork.ObjectId(TEST_IDS.LISTING_1),
SHARER_1: new MongooseSeedwork.ObjectId(TEST_IDS.SHARER_1),
RESERVATION_1: new MongooseSeedwork.ObjectId(TEST_IDS.RESERVATION_1),
} as const;
function makeActiveReserverFilter(userId: string) {
return {
reserver: new MongooseSeedwork.ObjectId(userId),
state: { $in: ['Accepted', 'Requested'] },
};
}
function makePastReserverFilter(userId: string) {
return {
reserver: new MongooseSeedwork.ObjectId(userId),
state: { $in: ['Cancelled', 'Closed', 'Rejected'] },
};
}
```
Then your expectations become shorter and more readable:
```ts
expect(mockDataSource.find).toHaveBeenCalledWith(
makeActiveReserverFilter(TEST_IDS.USER_1),
{ populateFields: ['listing', 'reserver'] },
);
expect(mockDataSource.find).toHaveBeenCalledWith(
{ reserver: TEST_OBJECT_IDS.USER_1 },
undefined,
);
```
This removes nested `new MongooseSeedwork.ObjectId(...)` noise across scenarios and makes it easier to see the intent of each test.
2) Add small assertion helpers instead of repeating `toHaveBeenCalledWith`
You can also wrap repeated patterns like `find` / `findOne` expectations:
```ts
function expectFindCalledWith(filter: unknown, options?: unknown) {
expect(mockDataSource.find).toHaveBeenCalledWith(filter, options);
}
function expectFindOneCalledWith(filter: unknown, options?: unknown) {
expect(mockDataSource.findOne).toHaveBeenCalledWith(filter, options);
}
function expectConverterCalledWithDoc(doc: Models.ReservationRequest.ReservationRequest) {
expect(mockConverter.toDomain).toHaveBeenCalledWith(doc, passport);
}
```
Then scenarios reduce to:
```ts
Then('I should receive an array of ReservationRequest entities', () => {
expectFindCalledWith({}, undefined);
});
And('the array should contain all reservation requests', () => {
expectConverterCalledWithDoc(mockReservationRequestDoc);
});
```
This keeps all your current assertions but avoids repeating the raw call signatures everywhere.
3) Prefer behavior assertions in most scenarios; keep filter checks in a few
Today almost every scenario asserts the exact filter passed into the data source. This is useful in a couple of focused tests, but in most scenarios it is enough (and less brittle) to assert on the returned domain entities.
For example, instead of:
```ts
When('I call getByReserverId with "user-1"', async () => {
await repository.getByReserverId(TEST_IDS.USER_1);
});
Then('I should receive an array of ReservationRequest entities', () => {
expect(mockDataSource.find).toHaveBeenCalledWith(
{ reserver: new MongooseSeedwork.ObjectId(TEST_IDS.USER_1) },
undefined,
);
});
And('the array should contain reservation requests where reserver is "user-1"', () => {
expect(mockConverter.toDomain).toHaveBeenCalledWith(mockReservationRequestDoc, passport);
});
```
capture the result and assert behavior:
```ts
let result: Domain.Contexts.ReservationRequest.ReservationRequest.ReservationRequestEntityReference[];
When('I call getByReserverId with "user-1"', async () => {
result = await repository.getByReserverId(TEST_IDS.USER_1);
});
Then('I should receive an array of ReservationRequest entities', () => {
expect(Array.isArray(result)).toBe(true);
expect(result.length).toBeGreaterThan(0);
});
And('the array should contain reservation requests where reserver is "user-1"', () => {
expect(result.every(r => String(r.reserver) === TEST_IDS.USER_1)).toBe(true);
});
```
Then keep only 1–2 dedicated scenarios that verify filter construction via the `expectFindCalledWith`/`expectFindOneCalledWith` helpers. This preserves your functional coverage but shifts most tests away from internal wiring.
4) Simplify constructor mocking by using a simple fake instance
You don’t have to mock the constructors for every scenario; you can construct the repository with a simple fake that matches the data source interface.
Instead of:
```ts
// Mock the constructors
vi.mocked(ReservationRequestDataSourceImpl).mockImplementation(
() => mockDataSource as unknown as InstanceType<typeof ReservationRequestDataSourceImpl>
);
vi.mocked(ReservationRequestConverter).mockImplementation(
() => mockConverter as unknown as ReservationRequestConverter
);
repository = new ReservationRequestReadRepositoryImpl(models, passport);
```
you can expose injection points in the tests only (without touching production) by constructing the data source/convertor directly when instantiating the repository (if the repository constructor allows it), or by wrapping the repository creation in a helper:
```ts
function makeRepositoryWithMocks() {
const dataSource = mockDataSource as unknown as ReservationRequestDataSourceImpl;
const converter = mockConverter as unknown as ReservationRequestConverter;
// If your repository allows passing collaborators directly:
return new ReservationRequestReadRepositoryImpl(models, passport, dataSource, converter);
}
```
If the constructor signature doesn’t currently support this, you can keep the constructor mocking but still wrap it in a helper to avoid repeating the same `mockImplementation` logic in `BeforeEachScenario`.
Either way, centralizing this setup into `makeRepositoryWithMocks()` (or similar) makes each scenario focus on “when I call X” and “then I get Y”, instead of on how the mocks are wired.
5) Reduce repeated converter assertions
Right now almost every scenario asserts:
```ts
expect(mockConverter.toDomain).toHaveBeenCalledWith(mockReservationRequestDoc, passport);
```
You can:
- have 1–2 scenarios dedicated to verifying the converter integration (called once per doc, with the right passport), and
- in other scenarios, assert only on the returned entities (as shown in point 3), trusting that the converter integration is already covered.
If you want to keep some safety in the other scenarios, you can use a lighter check:
```ts
expect(mockConverter.toDomain).toHaveBeenCalled();
```
instead of repeating the full argument structure.
These changes keep the new data‑source + converter design, but make the test suite less verbose, less implementation‑coupled, and easier to maintain.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
...readonly/reservation-request/reservation-request/reservation-request.read-repository.test.ts
Show resolved
Hide resolved
jasonmorais
reviewed
Nov 24, 2025
Contributor
jasonmorais
left a comment
jasonmorais
left a comment
There was a problem hiding this comment.
Basically fixing the event integration and then testing it works properly along with fixing the other commetns i left.
packages/sthrift/event-handler/src/handlers/integration/reservation-request-created.ts
Outdated
Show resolved
Hide resolved
...ft/domain/src/domain/contexts/reservation-request/reservation-request/reservation-request.ts
Outdated
Show resolved
Hide resolved
packages/cellix/transactional-email-service/src/transactional-email-service.ts
Outdated
Show resolved
Hide resolved
added 6 commits
December 18, 2025 00:27
added 2 commits
December 22, 2025 20:47
…pilot/refactor-service-sendgrid-facade
jasonmorais
reviewed
Dec 23, 2025
Contributor
jasonmorais
left a comment
jasonmorais
left a comment
There was a problem hiding this comment.
Main thing is being careful of copilot suggestions. The aggregate file for reservation request keeps having patterns broken in it. Please try to follow cellix and our other files and the way they access information
...ft/domain/src/domain/contexts/reservation-request/reservation-request/reservation-request.ts
Outdated
Show resolved
Hide resolved
...ft/domain/src/domain/contexts/reservation-request/reservation-request/reservation-request.ts
Outdated
Show resolved
Hide resolved
...ft/domain/src/domain/contexts/reservation-request/reservation-request/reservation-request.ts
Outdated
Show resolved
Hide resolved
...ft/domain/src/domain/contexts/reservation-request/reservation-request/reservation-request.ts
Outdated
Show resolved
Hide resolved
...ft/domain/src/domain/contexts/reservation-request/reservation-request/reservation-request.ts
Outdated
Show resolved
Hide resolved
...ft/domain/src/domain/contexts/reservation-request/reservation-request/reservation-request.ts
Outdated
Show resolved
Hide resolved
...ources/domain/reservation-request/reservation-request/reservation-request.repository.test.ts
Show resolved
Hide resolved
jasonmorais
reviewed
Jan 6, 2026
Contributor
jasonmorais
left a comment
jasonmorais
left a comment
There was a problem hiding this comment.
Really just changes that need to be made in reservation-request.ts. Want to make sure we are following our common practices for aggregate files there/ After that we should be good.
...ft/domain/src/domain/contexts/reservation-request/reservation-request/reservation-request.ts
Show resolved
Hide resolved
...ft/domain/src/domain/contexts/reservation-request/reservation-request/reservation-request.ts
Outdated
Show resolved
Hide resolved
...ft/domain/src/domain/contexts/reservation-request/reservation-request/reservation-request.ts
Outdated
Show resolved
Hide resolved
...ft/domain/src/domain/contexts/reservation-request/reservation-request/reservation-request.ts
Outdated
Show resolved
Hide resolved
…pilot/refactor-service-sendgrid-facade
…thub.com/simnova/sharethrift into copilot/refactor-service-sendgrid-facade
- Extended SNYK-JS-QS-14724253 expiration from 2026-01-19 to 2026-07-19 - This vulnerability was set to expire in 11 days, causing build failures - QS is a transitive dependency in express, Apollo Server, and Docusaurus - Not exploitable in current usage pattern - Extended for 6 months (per security best practices)
jasonmorais
reviewed
Jan 8, 2026
Contributor
jasonmorais
left a comment
jasonmorais
left a comment
There was a problem hiding this comment.
One last small comment
...ft/domain/src/domain/contexts/reservation-request/reservation-request/reservation-request.ts
Outdated
Show resolved
Hide resolved
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Refactoring @sthrift/service-sendgrid to support pluggable email providers with mock for local development, and implementing ReservationRequestCreated event handler structure.
Status: Code Review Feedback Addressed ✅
All packages built successfully. Addressed code review feedback:
Architecture:
@cellix/transactional-email-service- Generic interface@sthrift/transactional-email-service-sendgrid- SendGrid implementation@sthrift/transactional-email-service-mock- Mock for local devReservationRequestCreatedOriginal prompt
This section details on the original issue you should resolve
<issue_title>Refactor @sthrift/service-sendgrid package to provide a ServiceSendGrid facade with mock for local development</issue_title>
<issue_description>## Problem
Developers currently rely on SendGrid for email template verification, which requires sending real emails and a valid API key. This makes local development and testing workflows cumbersome. This effort aligns with the overall direction we are heading for disconnected development, which allows local development to progress without reliance on any external integrations.
Public Interface Direction
The key consideration is that
@sthrift/transactional-email-serviceshould be just a package with a single interface in it. It has to be generic enough to support multiple 3rd party vendors. The publicly exposed interface for the facade should be minimal, hiding all proprietary details and specific datatypes that are part of the SendGrid library, exposing only the absolute simplest interface necessary to get the job done. The code within the facade should translate this simple public interface into the proprietary calls to the email provider's API. This ensures we can swap implementations of mail sending services without having to change the public interface or upstream logic—enabling a true plug-and-play approach (e.g., swapping in Azure Communication Services). We should take this approach for all of our 3rd party integrations going forward.We should start by defining the public interface first—such as:
@sthrift/transactional-email-service(only defines the facade interface; the rest of the system refers only to this interface)Then, the specific implementations are fully interchangeable and can be swapped without impacting upstream code:
@sthrift/transactional-email-service-sendgrid-v3(facade-implementation)@sthrift/transactional-email-service-sendgrid-v4(facade-implementation)@sthrift/transactional-email-service-azure-communication(facade-implementation, example only)@sthrift/transactional-email-service-mock(facade-implementation)For this task, only concrete implementations for transactional email service sendgrid and transactional email service mock are required. Azure Communication Services is provided as an example of how this architecture enables easily swappable integrations and may be explored in the future.
Configuration Approach
The
@sthrift/apipackage will be responsible for determining which facade implementation is actually registered at application startup. This should be determined by environment variable values.Proposal
Refactor the existing
@sthrift/service-sendgridpackage so it becomes the facade for ServiceSendGrid. This facade should:SENDGRID_API_KEYenvironment variable. If the environment variable is set for local development, the mock implementation should be used; if set for production/remote, the actual SendGrid service should be used.Mock Implementation
tmp/inside the service-sendgrid package)..gitignoreso downloaded templates are not committed.Acceptance Criteria
@sthrift/transactional-email-servicepackage provides a minimal, generic interface for sending transactional emails, hiding all proprietary details.@sthrift/transactional-email-service-sendgrid-v3(or v4) as the SendGrid implementation@sthrift/transactional-email-service-mockas the mock implementationtmp/and never sends to SendGrid.@sthrift/apipackage determines which implementation is registered at app startup via environment variables.Related Code & References
sendgrid.ts- Main SendGrid class](https://githu...✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.
Summary by Sourcery
Introduce a provider-agnostic transactional email facade with SendGrid and local mock implementations, wire it into the API and event-handling pipeline, and emit an integration event when reservation requests are created to notify listing owners by email.
New Features:
Enhancements:
Build:
Tests: