fix(j-s): Whitelist config #16974
fix(j-s): Whitelist config #16974
Conversation
WalkthroughThe changes in this pull request involve modifications to the email notification handling within the Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
apps/judicial-system/backend/src/app/modules/notification/notification.config.ts (1)
8-9: Consider improving configuration clarity and documentationThe current implementation has several points that could benefit from improvement:
- The default value 'test' for CONTENTFUL_ENVIRONMENT might be too specific
- The inverse logic (!== 'master') makes the intent less clear
- The configuration lacks documentation explaining the whitelist behavior
Consider this alternative implementation for better clarity:
- shouldUseWhitelist: - env.required('CONTENTFUL_ENVIRONMENT', 'test') !== 'master', + // Controls email whitelist functionality + // Whitelist is disabled only in production (master) environment + shouldUseWhitelist: env.optional('CONTENTFUL_ENVIRONMENT') !== 'master' ?? true,Please add documentation in the README or configuration guide explaining:
- The purpose of email whitelisting
- When it's enabled/disabled
- How to configure it in different environments
apps/judicial-system/backend/src/app/modules/notification/baseNotification.service.ts (1)
92-94: Enhance type safety for configurationPer the coding guidelines for TypeScript safety, consider adding type constraints for the config interface.
Add type definitions:
interface NotificationConfig { shouldUseWhitelist: boolean; email: { fromName: string; fromEmail: string; replyToName: string; replyToEmail: string; }; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
apps/judicial-system/backend/src/app/modules/notification/baseNotification.service.ts(1 hunks)apps/judicial-system/backend/src/app/modules/notification/notification.config.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
apps/judicial-system/backend/src/app/modules/notification/baseNotification.service.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
apps/judicial-system/backend/src/app/modules/notification/notification.config.ts (1)
Pattern apps/**/*: "Confirm that the code adheres to the following:
- NextJS best practices, including file structure, API routes, and static generation methods.
- Efficient state management and server-side rendering techniques.
- Optimal use of TypeScript for component and utility type safety."
🔇 Additional comments (2)
apps/judicial-system/backend/src/app/modules/notification/notification.config.ts (1)
8-9: Verify the whitelist configuration usage across the application
The change from NODE_ENV to CONTENTFUL_ENVIRONMENT for determining whitelist behavior is significant. Let's verify its usage.
✅ Verification successful
The whitelist configuration change is correctly implemented and used
The verification shows that the shouldUseWhitelist configuration is properly integrated into the notification system:
- The configuration is used in
baseNotification.service.tsto control email filtering:if (!this.config.shouldUseWhitelist && recipients) { recipients = await this.handleWhitelist(recipients) }
- The whitelist implementation is robust with proper domain and email filtering through
filterWhitelistEmails - The change from NODE_ENV to CONTENTFUL_ENVIRONMENT aligns with the system's configuration pattern, where
masterenvironment represents production
The inverse logic (!== 'master') is intentional and correctly implements the requirement that whitelist should be used in all environments except master.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how the whitelist configuration is used across the codebase
# Find all references to shouldUseWhitelist
rg -A 3 "shouldUseWhitelist"
# Find any remaining references to production flag in notification contexts
rg -A 3 "production.*notification"
Length of output: 1034
Script:
#!/bin/bash
# Let's check the handleWhitelist implementation and any whitelist-related code
ast-grep --pattern 'handleWhitelist($_) {
$$$
}'
# Also check for any whitelist-related configuration or constants
rg -A 5 "whitelist" -i
Length of output: 32169
apps/judicial-system/backend/src/app/modules/notification/baseNotification.service.ts (1)
92-94: Verify whitelist configuration tests
Let's ensure this critical change is properly tested and consistently used across the codebase.
✅ Verification successful
Whitelist implementation is properly tested and configured
The whitelist functionality is well-implemented with:
- Configuration based on environment (
CONTENTFUL_ENVIRONMENT !== 'master') - Comprehensive test coverage in
formatters.spec.ts - Proper domain and email whitelisting with clear default values
- Appropriate error handling and logging
The implementation is consistent across the codebase with no conflicting usages found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for tests covering whitelist functionality
rg -t typescript -t javascript "shouldUseWhitelist.*test" apps/judicial-system
# Search for other usages of shouldUseWhitelist
rg -t typescript -t javascript "shouldUseWhitelist" apps/judicial-system
# Search for potential legacy uses of production flag in email context
rg -t typescript -t javascript "production.*email" apps/judicial-system
Length of output: 332
Script:
#!/bin/bash
# Search for tests covering whitelist functionality without file type filters
rg "shouldUseWhitelist.*test" apps/judicial-system
# Search for other usages of shouldUseWhitelist
rg "shouldUseWhitelist" apps/judicial-system
# Search for whitelist-related tests
rg "handleWhitelist" apps/judicial-system
Length of output: 696
Script:
#!/bin/bash
# Let's check the notification config implementation
cat apps/judicial-system/backend/src/app/modules/notification/notification.config.ts
# Check for any tests related to notification service
rg -l "notification.*\.spec\." apps/judicial-system
# Check the implementation of handleWhitelist method
rg -A 10 "handleWhitelist" apps/judicial-system/backend/src/app/modules/notification/baseNotification.service.ts
Length of output: 2182
Script:
#!/bin/bash
# Let's check the notifications constants for whitelist configuration
rg -A 5 "emailWhitelist" apps/judicial-system
# Check for any test files in the notification module directory
fd "\.spec\.(ts|js)$" apps/judicial-system/backend/src/app/modules/notification/
# Check the filterWhitelistEmails implementation
rg -A 10 "filterWhitelistEmails" apps/judicial-system
Length of output: 16619
apps/judicial-system/backend/src/app/modules/notification/baseNotification.service.ts
Outdated
Show resolved
Hide resolved
Datadog ReportAll test runs ✅ 15 Total Test Services: 0 Failed, 13 Passed Test ServicesThis report shows up to 10 services
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #16974 +/- ##
=======================================
Coverage 35.62% 35.62%
=======================================
Files 6914 6914
Lines 145990 145990
Branches 41446 41446
=======================================
Hits 52011 52011
Misses 93979 93979
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Sentry.
|
* fix(j-s): Whitelist config * Update baseNotification.service.ts --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
What
Fix whitelist config
Why
Misunderstanding of the node env variable
Checklist:
Summary by CodeRabbit
New Features
shouldUseWhitelistto control email whitelist behavior based on the environment.Bug Fixes