Skip to content

BCC phase change emails#31

Merged
jmgasper merged 1 commit into
masterfrom
develop
Mar 11, 2026
Merged

BCC phase change emails#31
jmgasper merged 1 commit into
masterfrom
develop

Conversation

@jmgasper

Copy link
Copy Markdown
Contributor

No description provided.

@jmgasper jmgasper merged commit 5a7132e into master Mar 11, 2026
4 of 6 checks passed
} as unknown as jest.Mocked<HttpService>;

configService = {
get: jest.fn((key: string) => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[⚠️ maintainability]
The configService.get method uses a switch statement to return configuration values. Consider using a configuration object or map to store these values for easier maintenance and scalability.

expect(message.topic).toBe('external.action.email');
expect(message.payload.from).toBe('no-reply@topcoder.com');
expect(message.payload.replyTo).toBe('no-reply@topcoder.com');
expect(message.payload.recipients).toEqual(['no-reply@topcoder.com']);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[⚠️ correctness]
The recipients field is set to ['no-reply@topcoder.com']. Ensure this is intentional and aligns with the business requirements, as it might be expected to contain actual recipient addresses.

from: defaultNotificationEmail,
replyTo: defaultNotificationEmail,
recipients: recipientEmails,
recipients: [defaultNotificationEmail],

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[⚠️ design]
Changing the recipients field to always include defaultNotificationEmail and moving actual recipient emails to bcc is a significant change. Ensure that this aligns with the intended behavior and that all stakeholders are aware of this change, as it affects how emails are sent and received.

if (Number.isNaN(date.getTime())) {
this.logger.warn(
`Unable to format phase date "${value}" for phase change notification payload.`,
`Unable to format phase date "${String(value)}" for phase change notification payload.`,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[💡 correctness]
Using String(value) ensures that the value is converted to a string, which is safer than relying on implicit conversion. However, ensure that this change does not affect any existing logging or error handling logic that might depend on the original format of value.

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