fix(j-s): Whitelist emails #16907
fix(j-s): Whitelist emails #16907
Conversation
WalkthroughThe pull request introduces a new function, Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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: 9
🧹 Outside diff range and nitpick comments (30)
apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/sendIndictmentReturnedNotifications.spec.ts (2)
75-78: Consider adding type assertion for prosecutor propertiesThe case setup looks good, but consider adding type safety for the prosecutor object to prevent potential runtime errors.
- prosecutor: { name: prosecutor.name, email: prosecutor.email }, + prosecutor: { name: prosecutor.name as string, email: prosecutor.email as string },
Line range hint
87-91: Replace hardcoded localhost URL with environment configurationThe test contains a hardcoded localhost URL in the email content. Consider using an environment-aware configuration for the base URL.
- html: `${courtName} hefur endursent ákæru vegna lögreglumáls ${policeCaseNumbers[0]}. Þú getur nálgast samantekt málsins á <a href="http://localhost:4200/akaera/stadfesta/${caseId}">yfirlitssíðu málsins í Réttarvörslugátt.</a>`, + html: `${courtName} hefur endursent ákæru vegna lögreglumáls ${policeCaseNumbers[0]}. Þú getur nálgast samantekt málsins á <a href="${config.baseUrl}/akaera/stadfesta/${caseId}">yfirlitssíðu málsins í Réttarvörslugátt.</a>`,apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/sendDefendantsNotUpdatedAtCourtNotifications.spec.ts (1)
83-86: Consider extracting email templates to constantsThe email subject and content templates could be moved to a separate constants file for better maintainability and reuse across tests.
+ // In a new file: apps/judicial-system/backend/src/app/modules/notification/constants.ts + export const EMAIL_TEMPLATES = { + DEFENDANTS_NOT_UPDATED: { + SUBJECT: 'Skráning varnaraðila/verjenda í máli %s', + BODY: 'Ekki tókst að skrá varnaraðila/verjendur í máli %s í Auði. Yfirfara þarf málið í Auði og skrá rétta aðila áður en því er lokað.' + } + } - subject: `Skráning varnaraðila/verjenda í máli ${courtCaseNumber}`, - html: `Ekki tókst að skrá varnaraðila/verjendur í máli ${courtCaseNumber} í Auði. Yfirfara þarf málið í Auði og skrá rétta aðila áður en því er lokað.`, + subject: format(EMAIL_TEMPLATES.DEFENDANTS_NOT_UPDATED.SUBJECT, courtCaseNumber), + html: format(EMAIL_TEMPLATES.DEFENDANTS_NOT_UPDATED.BODY, courtCaseNumber),apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/sendRevokedNotifications.spec.ts (2)
40-48: Consider adding TypeScript interfaces for test data structureThe case setup with test users is well-organized. To enhance maintainability and type safety, consider defining interfaces for the test user and case data structures.
interface TestUser { name: string; email: string; nationalId?: string; } interface TestCase { id: string; judge: Pick<TestUser, 'name' | 'email'>; registrar: Pick<TestUser, 'name' | 'email'>; defendants: Array<{ defenderNationalId: string; defenderName: string; defenderEmail: string; }>; // ... other properties }
96-96: Consider parameterizing email content testsThe email recipient updates and content verification look good. To make the tests more maintainable, consider extracting the email content templates into test constants or fixtures.
const EMAIL_TEMPLATES = { REVOKED_JUDGE: (caseNumber: string, office: string) => `${office} hefur afturkallað ákæru í máli ${caseNumber}...`, // ... other templates };Also applies to: 103-103, 110-110, 119-121
apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/sendAppealJudgesAssignedNotifications.spec.ts (3)
27-32: Consider using constants for user typesWhile the test user creation is clean and maintainable, consider defining constants for the user types to prevent typos and improve maintainability.
+const USER_TYPES = { + JUDGE_1: 'judge1', + JUDGE_2: 'judge2', + JUDGE_3: 'judge3', + ASSISTANT: 'assistant', +} as const; -const { judge1, judge2, judge3, assistant } = createTestUsers([ - 'judge1', - 'judge2', - 'judge3', - 'assistant', -]) +const { judge1, judge2, judge3, assistant } = createTestUsers([ + USER_TYPES.JUDGE_1, + USER_TYPES.JUDGE_2, + USER_TYPES.JUDGE_3, + USER_TYPES.ASSISTANT, +])
58-78: Consider reducing repetition in judge objectsThe judge objects follow the same structure. Consider creating a helper function to reduce duplication.
+const createJudgeObject = (judge: typeof judge1) => ({ + name: judge.name, + email: judge.email, + id: judge.id, + role: UserRole.COURT_OF_APPEALS_JUDGE, +}); -appealJudge1: { - name: judge1.name, - email: judge1.email, - id: judge1.id, - role: UserRole.COURT_OF_APPEALS_JUDGE, -}, +appealJudge1: createJudgeObject(judge1),
Line range hint
103-131: Replace hardcoded URL with configuration valueThe test contains hardcoded URLs (
http://localhost:4200) in the email content. This could cause issues when running tests in different environments.Consider using a configuration value:
+const TEST_BASE_URL = process.env.TEST_BASE_URL || 'http://localhost:4200'; -html: `Landsréttur hefur skráð þig sem dómara í máli nr. ${appealCaseNumber}. Dómsformaður er ${judge1.name}. Þú getur nálgast yfirlit málsins á <a href="http://localhost:4200/landsrettur/yfirlit/${caseId}">yfirlitssíðu málsins í Réttarvörslugátt.</a>`, +html: `Landsréttur hefur skráð þig sem dómara í máli nr. ${appealCaseNumber}. Dómsformaður er ${judge1.name}. Þú getur nálgast yfirlit málsins á <a href="${TEST_BASE_URL}/landsrettur/yfirlit/${caseId}">yfirlitssíðu málsins í Réttarvörslugátt.</a>`,apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/sendAppealFilesUpdatedNotifications.spec.ts (2)
27-32: Consider using typed constants for user roles.While the
createTestUsersutility improves maintainability, consider defining the user types as typed constants to enhance type safety and prevent typos.+const USER_TYPES = { + ASSISTANT: 'assistant', + JUDGE_1: 'judge1', + JUDGE_2: 'judge2', + JUDGE_3: 'judge3', +} as const; -const { assistant, judge1, judge2, judge3 } = createTestUsers([ - 'assistant', - 'judge1', - 'judge2', - 'judge3', -]) +const { assistant, judge1, judge2, judge3 } = createTestUsers([ + USER_TYPES.ASSISTANT, + USER_TYPES.JUDGE_1, + USER_TYPES.JUDGE_2, + USER_TYPES.JUDGE_3, +])
Line range hint
104-132: Consider reducing duplication in email content verification.The test contains repeated HTML content and a hardcoded localhost URL. Consider these improvements:
- Extract common email content to a constant
- Use environment configuration for the base URL
+const BASE_URL = process.env.APP_URL || 'http://localhost:4200' +const getEmailContent = (courtCaseNumber: string, appealCaseNumber: string, caseId: string) => + `Ný gögn hafa borist vegna kæru í máli ${courtCaseNumber} (Landsréttarmál nr. ${appealCaseNumber}). ` + + `Hægt er að nálgast gögn málsins á <a href="${BASE_URL}/landsrettur/yfirlit/${caseId}">yfirlitssíðu málsins í Réttarvörslugátt</a>.` expect(mockEmailService.sendEmail).toHaveBeenCalledWith( expect.objectContaining({ to: [{ name: assistant.name, address: assistant.email }], subject: `Ný gögn í máli ${courtCaseNumber} (${appealCaseNumber})`, - html: `Ný gögn hafa borist vegna kæru í máli ${courtCaseNumber} (Landsréttarmál nr. ${appealCaseNumber}). Hægt er að nálgast gögn málsins á <a href="http://localhost:4200/landsrettur/yfirlit/${caseId}">yfirlitssíðu málsins í Réttarvörslugátt</a>.`, + html: getEmailContent(courtCaseNumber, appealCaseNumber, caseId), }), )apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/sendIndictmentsWaitingForConfirmationNotifications.spec.ts (1)
24-27: Consider using an enum or constants for prosecutor identifiers.While the
createTestUsersutility improves maintainability, using string literals for user identifiers could lead to typos or inconsistencies. Consider defining these identifiers as constants or an enum.+const PROSECUTOR_IDS = { + PROSECUTOR_1: 'prosecutor1', + PROSECUTOR_2: 'prosecutor2', +} as const; -const { prosecutor1, prosecutor2 } = createTestUsers([ - 'prosecutor1', - 'prosecutor2', -]) +const { prosecutor1, prosecutor2 } = createTestUsers([ + PROSECUTOR_IDS.PROSECUTOR_1, + PROSECUTOR_IDS.PROSECUTOR_2, +])apps/judicial-system/backend/src/app/modules/notification/test/createTestingNotificationModule.ts (1)
39-61: Consider enhancing the test data generationWhile the function provides a good foundation for creating test users, there are several improvements that could make it more robust:
- The static nationalId ('1234567890') could mask bugs related to unique identification in production. Consider generating unique nationalIds.
- The email domain is hardcoded. Consider making it configurable or moving it to a constant.
- The roles parameter could be more strictly typed using a union type of valid roles.
Here's a suggested improvement:
+ const TEST_EMAIL_DOMAIN = 'omnitrix.is' + type ValidRole = 'judge' | 'prosecutor' | 'registrar' | 'admin' // add your valid roles export const createTestUsers = ( - roles: string[], + roles: ValidRole[], ): Record< - string, + ValidRole, { id: string name: string email: string mobile: string nationalId: string } > => roles.reduce((acc, role) => { const id = uuid() + const nationalId = Math.floor(Math.random() * 9000000000 + 1000000000).toString() acc[role] = { id: id, name: `${role}-${id}`, - email: `${role}-${id}@omnitrix.is`, + email: `${role}-${id}@${TEST_EMAIL_DOMAIN}`, mobile: id, - nationalId: '1234567890', + nationalId, } return acc - }, {} as Record<string, { id: string; name: string; email: string; mobile: string; nationalId: string }>) + }, {} as Record<ValidRole, { id: string; name: string; email: string; mobile: string; nationalId: string }>)Consider adding unit tests for this utility function to ensure it behaves correctly with various inputs.
apps/judicial-system/backend/src/app/modules/notification/baseNotification.service.ts (1)
56-77: Add input validation and improve error logging.The method implementation could be enhanced in several ways:
- Add input validation for the recipients array
- Make warning messages more descriptive by including the filtered/missing email addresses
- Add JSDoc documentation to describe the method's purpose and parameters
Consider applying these improvements:
+ /** + * Filters email recipients based on configured whitelists in non-production environments. + * @param recipients - Array of email addresses to filter + * @returns Filtered array of whitelisted email addresses + */ private async handleWhitelist(recipients: string[]): Promise<string[]> { + if (!recipients?.length) { + return []; + } const whitelist = this.formatMessage(notifications.emailWhitelist) const whitelistDomains = this.formatMessage( notifications.emailWhitelistDomains, ) const whitelistedEmails = filterWhitelistEmails( recipients, whitelistDomains, whitelist, ) if (whitelistedEmails.length === 0) { - this.logger.warn('No whitelisted emails found in recipients') + this.logger.warn('No whitelisted emails found in recipients', { + originalRecipients: recipients, + whitelist, + whitelistDomains + }) } if (whitelistedEmails.length !== recipients?.length) { - this.logger.warn('Some emails missing from whitelist') + this.logger.warn('Some emails missing from whitelist', { + filteredOut: recipients.filter(email => !whitelistedEmails.includes(email)) + }) } return whitelistedEmails }apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/sendCaseFilesUpdatedNotifications.spec.ts (4)
29-34: Consider adding type safety to createTestUsers parametersWhile the implementation is clean, consider adding a type for the user role strings to prevent typos and ensure type safety.
- createTestUsers([ - 'prosecutor', - 'judge', - 'defender', - 'spokesperson', - ]) + createTestUsers<UserRole>([ + UserRole.PROSECUTOR, + UserRole.JUDGE, + UserRole.DEFENDER, + UserRole.SPOKESPERSON, + ])
58-72: Consider extracting test case data setupThe case data setup is mixed with the test logic. Consider extracting it to a separate helper function for better maintainability and reusability.
const createTestCase = (users: ReturnType<typeof createTestUsers>) => ({ id: caseId, type: CaseType.INDICTMENT, courtCaseNumber, court: { name: 'Héraðsdómur Reykjavíkur' }, prosecutor: { name: users.prosecutor.name, email: users.prosecutor.email }, judge: { name: users.judge.name, email: users.judge.email }, defendants: [{ defenderNationalId: users.defender.nationalId, defenderName: users.defender.name, defenderEmail: users.defender.email, }], civilClaimants: [{ hasSpokesperson: true, spokespersonNationalId: users.spokesperson.nationalId, spokespersonName: users.spokesperson.name, spokespersonEmail: users.spokesperson.email, }], }) as Case;
Line range hint
101-153: Enhance test assertions for more robust verificationWhile the current assertions verify the basic functionality, consider adding these improvements:
- Verify the exact number of email calls to ensure no unexpected notifications
- Test the negative case where notification should not be sent
- Add assertions for email template consistency
// Add at the start of each test expect(mockEmailService.sendEmail).toHaveBeenCalledTimes(3); // Add negative test case describe('when user has no email', () => { beforeEach(async () => { const usersWithoutEmail = createTestUsers(['prosecutor']); usersWithoutEmail.prosecutor.email = ''; then = await givenWhenThen(usersWithoutEmail.prosecutor); }); it('should skip sending notification', () => { expect(mockEmailService.sendEmail).not.toHaveBeenCalled(); expect(then.result).toEqual({ delivered: false }); }); });
Line range hint
29-153: Add tests for email whitelisting functionalityGiven that the PR's objective is to implement email whitelisting for development and staging environments, the test suite should include scenarios that verify:
- Emails are only sent to whitelisted addresses in non-production environments
- Whitelisting rules are correctly applied for different user roles
- Behavior when mixing whitelisted and non-whitelisted email addresses
Would you like me to help create test cases for the whitelisting functionality?
apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/defendantNotification/sendDefenderAssignedNotifications.spec.ts (1)
42-43: LGTM! Consider type safety improvement.The test data setup using
createTestUsersis clean and maintainable.Consider using a TypeScript enum or const array for role types to prevent typos and improve maintainability:
const VALID_ROLES = ['defender', 'prosecutor', 'judge'] as const; type Role = typeof VALID_ROLES[number]; const { defender } = createTestUsers([VALID_ROLES[0]]);apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/sendAppealReceivedByCourtNotifications.spec.ts (2)
63-70: Consider extracting hardcoded URLs to configurationThe test contains hardcoded URLs (e.g.,
http://localhost:4200). Consider extracting these into test configuration constants for better maintainability.+ const BASE_URL = process.env.BASE_URL || 'http://localhost:4200'Also applies to: 98-98, 110-110, 120-120, 133-133, 152-152, 162-162
Line range hint
104-107: Consider adding test cases for statement deadline calculationThe tests verify the presence of the statement deadline in notifications but don't explicitly verify the calculation logic. Consider adding test cases that verify different deadline scenarios.
Example test case to add:
it('should calculate correct statement deadline', () => { const receivedDate = new Date('2024-01-01T10:00:00Z') const expectedDeadline = formatDate( getStatementDeadline(receivedDate), 'PPPp' ) // Assert the exact expected formatted date expect(expectedDeadline).toBe('expected formatted date') })Also applies to: 114-117, 123-126
apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/sendCourtDateNotifications.spec.ts (1)
9-9: Good improvement in test setup, but consider further enhancements.The introduction of
createTestUsersimproves maintainability. However, consider extracting common test data setup (likecourtName,courtCaseNumber) into a shared helper function as well.// Consider creating a helper like: function createTestCaseData() { return { courtName: 'Héraðsdómur Reykjavíkur', courtCaseNumber: uuid(), ...createTestUsers(['prosecutor', 'defender']) } }Also applies to: 13-16, 42-42
apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/sendAppealToCourtOfAppealsNotifications.spec.ts (1)
47-48: Consider using JSON.stringify for environment variables.While the current approach works, using template literals for JSON strings could be error-prone. Consider using
JSON.stringifyfor better reliability:- process.env.COURTS_ASSISTANT_MOBILE_NUMBERS = `{"${court.id}": "${court.mobile}"}` - process.env.COURTS_EMAILS = `{"${court.id}": "${court.email}"}` + process.env.COURTS_ASSISTANT_MOBILE_NUMBERS = JSON.stringify({ + [court.id]: court.mobile, + }) + process.env.COURTS_EMAILS = JSON.stringify({ + [court.id]: court.email, + })apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/sendAppealCompletedNotifications.spec.ts (2)
52-52: Document the UUID mapping for COURTS_EMAILS.While the environment variable setup is correct, consider adding a comment explaining the significance of the UUID
4676f08b-aab4-4b4f-a366-697540788088in the COURTS_EMAILS mapping to improve maintainability.
Line range hint
107-190: Consider using environment-specific URLs in email templates.The test uses hardcoded URLs:
http://localhost:4200for some notificationshttps://rettarvorslugatt.island.isfor othersConsider using configuration values for these URLs to ensure consistency across different environments (development, staging, production).
Example approach:
const baseUrl = mockConfig.environment === 'production' ? 'https://rettarvorslugatt.island.is' : 'http://localhost:4200';apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/sendAppealStatementNotifications.spec.ts (2)
12-15: LGTM! Consider enhancing type safety for roles.The introduction of
createTestUsersutility and centralized role management improves maintainability.Consider adding a type definition for the roles to prevent typos and ensure consistency:
type TestUserRole = 'prosecutor' | 'defender' | 'assistant' | 'judge1' | 'judge2' | 'judge3'; const roles: TestUserRole[] = [ 'prosecutor', 'defender', 'assistant', 'judge1', 'judge2', 'judge3', ];Also applies to: 32-43
116-119: Consider reducing duplication in court email assertions.The same block of court email assertions is repeated across multiple test scenarios.
Consider extracting these assertions into a helper function:
const assertCourtEmails = () => { expectCourtEmail(assistant.name, assistant.email); expectCourtEmail(judge1.name, judge1.email); expectCourtEmail(judge2.name, judge2.email); expectCourtEmail(judge3.name, judge3.email); }; // Usage in tests it('should send notification to appeals court and defender', () => { assertCourtEmails(); // ... rest of the test });Also applies to: 146-149, 173-176
apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/sendAppealWithdrawnNotifications.spec.ts (1)
133-135: Consider environment-aware URL configurationThe notification URLs are hardcoded to
https://rettarvorslugatt.island.is. Consider using an environment variable or configuration value to make the URL environment-aware.- html: `...Hægt er að nálgast yfirlitssíðu málsins á <a href="https://rettarvorslugatt.island.is">rettarvorslugatt.island.is</a>.`, + html: `...Hægt er að nálgast yfirlitssíðu málsins á <a href="${config.PORTAL_URL}">${config.PORTAL_DOMAIN}</a>.`,Also applies to: 157-159, 182-184, 206-208, 215-216, 231-234, 243-246, 253-256
apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/sendRulingNotifications.spec.ts (1)
91-94: Consider extracting repeated prosecutor object creationThe prosecutor object is created multiple times with the same pattern. Consider extracting this into a helper function to reduce duplication.
+ const createProsecutorFromTestUser = (testUser: any) => ({ + name: testUser.name, + email: testUser.email, + }); - const prosecutor = { - name: testProsecutor.name, - email: testProsecutor.email, - } + const prosecutor = createProsecutorFromTestUser(testProsecutor);Also applies to: 123-126, 156-159
apps/judicial-system/backend/src/app/formatters/formatters.ts (1)
806-832: LGTM! The implementation is efficient and handles the basic cases well.The function efficiently filters emails using Sets for O(1) lookups and handles empty inputs correctly.
Consider adding basic input validation and error handling:
export const filterWhitelistEmails = ( emails: string[], domainWhitelist: string, emailWhitelist: string, ) => { - if (!emails || emails.length === 0) return [] + if (!emails?.length) return [] + if (typeof domainWhitelist !== 'string' || typeof emailWhitelist !== 'string') { + throw new Error('Domain and email whitelists must be strings') + } const allowedDomains = new Set( domainWhitelist .split(',') .map((d) => d.trim()) .filter(Boolean), ) const allowedEmails = new Set( emailWhitelist .split(',') .map((e) => e.trim()) .filter(Boolean), ) return emails.filter((email) => { + if (typeof email !== 'string' || !email.includes('@')) return false const domain = email.split('@')[1] return ( domain && (allowedDomains.has(domain) || allowedEmails.has(email.trim())) ) }) }Consider adding email format validation using a regular expression:
export const filterWhitelistEmails = ( emails: string[], domainWhitelist: string, emailWhitelist: string, ) => { if (!emails?.length) return [] if (typeof domainWhitelist !== 'string' || typeof emailWhitelist !== 'string') { throw new Error('Domain and email whitelists must be strings') } + const emailRegex = /^[^\s@]+@[^\s@]+\.[^\s@]+$/ const allowedDomains = new Set( domainWhitelist .split(',') .map((d) => d.trim()) .filter(Boolean), ) const allowedEmails = new Set( emailWhitelist .split(',') .map((e) => e.trim()) .filter(Boolean), ) return emails.filter((email) => { - if (typeof email !== 'string' || !email.includes('@')) return false + if (typeof email !== 'string' || !emailRegex.test(email)) return false const domain = email.split('@')[1] return ( domain && (allowedDomains.has(domain) || allowedEmails.has(email.trim())) ) }) }apps/judicial-system/backend/src/app/formatters/formatters.spec.ts (1)
1987-2020: Good test coverage for basic scenarios, but consider adding more edge cases.The test suite provides good coverage for the main functionality of
filterWhitelistEmails. However, consider adding the following test cases to make it more robust:
- Test case sensitivity handling (e.g., TEST@rvg.is vs test@rvg.is)
- Test whitespace handling in whitelist (e.g., "email@domain.com, email2@domain.com" vs "email@domain.com,email2@domain.com")
- Test invalid email formats
- Test multiple domain whitelists (if supported)
- Test null/undefined inputs for defensive programming
Example test cases to add:
it('should handle case sensitivity correctly', () => { const emails = ['test@RVG.IS', 'test2@rvg.is'] const whitelist = '' const domainWhitelist = 'rvg.is' const result = filterWhitelistEmails(emails, domainWhitelist, whitelist) expect(result).toEqual(emails) }) it('should handle whitespace in whitelist', () => { const emails = ['test@rvg.is', 'test2@rvg.is'] const whitelist = ' test@rvg.is, test2@rvg.is ' const domainWhitelist = '' const result = filterWhitelistEmails(emails, domainWhitelist, whitelist) expect(result).toEqual(emails) }) it('should handle null/undefined inputs gracefully', () => { expect(filterWhitelistEmails(null, '', '')).toEqual([]) expect(filterWhitelistEmails(undefined, '', '')).toEqual([]) expect(filterWhitelistEmails([], null, '')).toEqual([]) expect(filterWhitelistEmails([], undefined, '')).toEqual([]) })
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (25)
apps/judicial-system/backend/src/app/formatters/formatters.spec.ts(2 hunks)apps/judicial-system/backend/src/app/formatters/formatters.ts(1 hunks)apps/judicial-system/backend/src/app/formatters/index.ts(1 hunks)apps/judicial-system/backend/src/app/messages/notifications.ts(1 hunks)apps/judicial-system/backend/src/app/modules/notification/baseNotification.service.ts(3 hunks)apps/judicial-system/backend/src/app/modules/notification/defendantNotification.service.ts(0 hunks)apps/judicial-system/backend/src/app/modules/notification/test/createTestingNotificationModule.ts(2 hunks)apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/defendantNotification/sendDefenderAssignedNotifications.spec.ts(4 hunks)apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/sendAdvocateAssignedNotifications.spec.ts(7 hunks)apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/sendAppealCompletedNotifications.spec.ts(7 hunks)apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/sendAppealFilesUpdatedNotifications.spec.ts(6 hunks)apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/sendAppealJudgesAssignedNotifications.spec.ts(5 hunks)apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/sendAppealReceivedByCourtNotifications.spec.ts(10 hunks)apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/sendAppealStatementNotifications.spec.ts(9 hunks)apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/sendAppealToCourtOfAppealsNotifications.spec.ts(6 hunks)apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/sendAppealWithdrawnNotifications.spec.ts(11 hunks)apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/sendCaseFilesUpdatedNotifications.spec.ts(5 hunks)apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/sendCourtDateNotifications.spec.ts(9 hunks)apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/sendDefendantsNotUpdatedAtCourtNotifications.spec.ts(4 hunks)apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/sendIndictmentDeniedNotifications.spec.ts(2 hunks)apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/sendIndictmentReturnedNotifications.spec.ts(4 hunks)apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/sendIndictmentsWaitingForConfirmationNotifications.spec.ts(3 hunks)apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/sendReadyForCourtNotifications.spec.ts(20 hunks)apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/sendRevokedNotifications.spec.ts(4 hunks)apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/sendRulingNotifications.spec.ts(7 hunks)
💤 Files with no reviewable changes (1)
- apps/judicial-system/backend/src/app/modules/notification/defendantNotification.service.ts
🧰 Additional context used
📓 Path-based instructions (24)
apps/judicial-system/backend/src/app/formatters/formatters.spec.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/formatters/formatters.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/formatters/index.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/messages/notifications.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/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/test/createTestingNotificationModule.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/test/internalNotificationController/defendantNotification/sendDefenderAssignedNotifications.spec.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/test/internalNotificationController/sendAdvocateAssignedNotifications.spec.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/test/internalNotificationController/sendAppealCompletedNotifications.spec.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/test/internalNotificationController/sendAppealFilesUpdatedNotifications.spec.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/test/internalNotificationController/sendAppealJudgesAssignedNotifications.spec.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/test/internalNotificationController/sendAppealReceivedByCourtNotifications.spec.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/test/internalNotificationController/sendAppealStatementNotifications.spec.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/test/internalNotificationController/sendAppealToCourtOfAppealsNotifications.spec.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/test/internalNotificationController/sendAppealWithdrawnNotifications.spec.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/test/internalNotificationController/sendCaseFilesUpdatedNotifications.spec.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/test/internalNotificationController/sendCourtDateNotifications.spec.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/test/internalNotificationController/sendDefendantsNotUpdatedAtCourtNotifications.spec.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/test/internalNotificationController/sendIndictmentDeniedNotifications.spec.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/test/internalNotificationController/sendIndictmentReturnedNotifications.spec.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/test/internalNotificationController/sendIndictmentsWaitingForConfirmationNotifications.spec.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/test/internalNotificationController/sendReadyForCourtNotifications.spec.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/test/internalNotificationController/sendRevokedNotifications.spec.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/test/internalNotificationController/sendRulingNotifications.spec.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."
📓 Learnings (5)
apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/sendCaseFilesUpdatedNotifications.spec.ts (1)
Learnt from: gudjong
PR: island-is/island.is#16452
File: apps/judicial-system/backend/src/app/modules/notification/internalNotification.service.ts:1845-1854
Timestamp: 2024-11-12T15:15:11.835Z
Learning: In the `sendCaseFilesUpdatedNotifications` method within `internalNotification.service.ts`, when the `user` is a defence user, it is intended to send notifications to the prosecutor.
apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/sendCourtDateNotifications.spec.ts (1)
Learnt from: gudjong
PR: island-is/island.is#16452
File: apps/judicial-system/backend/src/app/modules/notification/internalNotification.service.ts:1845-1854
Timestamp: 2024-11-12T15:15:11.835Z
Learning: In the `sendCaseFilesUpdatedNotifications` method within `internalNotification.service.ts`, when the `user` is a defence user, it is intended to send notifications to the prosecutor.
apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/sendIndictmentDeniedNotifications.spec.ts (1)
Learnt from: gudjong
PR: island-is/island.is#16452
File: apps/judicial-system/backend/src/app/modules/notification/internalNotification.service.ts:1845-1854
Timestamp: 2024-11-12T15:15:11.835Z
Learning: In the `sendCaseFilesUpdatedNotifications` method within `internalNotification.service.ts`, when the `user` is a defence user, it is intended to send notifications to the prosecutor.
apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/sendIndictmentReturnedNotifications.spec.ts (1)
Learnt from: gudjong
PR: island-is/island.is#16452
File: apps/judicial-system/backend/src/app/modules/notification/internalNotification.service.ts:1845-1854
Timestamp: 2024-11-12T15:15:11.835Z
Learning: In the `sendCaseFilesUpdatedNotifications` method within `internalNotification.service.ts`, when the `user` is a defence user, it is intended to send notifications to the prosecutor.
apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/sendReadyForCourtNotifications.spec.ts (1)
Learnt from: gudjong
PR: island-is/island.is#16452
File: apps/judicial-system/backend/src/app/modules/notification/internalNotification.service.ts:1845-1854
Timestamp: 2024-11-12T15:15:11.835Z
Learning: In the `sendCaseFilesUpdatedNotifications` method within `internalNotification.service.ts`, when the `user` is a defence user, it is intended to send notifications to the prosecutor.
🔇 Additional comments (46)
apps/judicial-system/backend/src/app/formatters/index.ts (2)
31-31: LGTM! Export addition follows the established pattern.
The new filterWhitelistEmails export is correctly placed with other formatting-related functions and follows the consistent naming convention.
31-31: Verify TypeScript types and notification system integration.
Since this is part of the notification system, let's verify:
- Type safety of the exported function
- Integration with existing notification flows
✅ Verification successful
Type safety and notification integration verified
The filterWhitelistEmails function is properly integrated with the notification system and has type safety:
- Function has clear type signature with string array inputs and returns string array
- Used correctly in BaseNotificationService for email whitelisting
- Integrated with email delivery flow in a type-safe manner
- Has comprehensive test coverage verifying functionality
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify TypeScript types and usage in notification system
# Check TypeScript type definitions
ast-grep --pattern 'function filterWhitelistEmails($params): $returnType {
$$$
}'
# Check usage in notification flows
rg -A 5 'filterWhitelistEmails' --type ts
# Check for potential type errors
rg 'filterWhitelistEmails.*any' --type ts
Length of output: 5749
Script:
#!/bin/bash
# Let's check the implementation and usage more thoroughly
# Check the complete implementation
cat apps/judicial-system/backend/src/app/formatters/formatters.ts
# Check the complete notification service implementation
cat apps/judicial-system/backend/src/app/modules/notification/baseNotification.service.ts
# Check for any interfaces or types related to this function
rg -t ts "interface.*Notification|type.*Notification" -A 3
Length of output: 140465
apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/sendIndictmentDeniedNotifications.spec.ts (2)
11-14: LGTM! Clean import organization
The new imports are well-structured and properly separate the testing utilities.
Line range hint 1-93: Verify test coverage for all notification scenarios
The test file should be comprehensive in covering the email notification behavior. Let's verify the test coverage:
✅ Verification successful
Email whitelisting is properly handled by the base notification service
The test file is correctly implemented and does not need modifications since:
- The test uses a mocked EmailService that inherits from BaseNotificationService which already handles email whitelisting
- The BaseNotificationService implementation shows that email whitelisting is handled in the
handleWhitelistmethod which is called in non-production environments - The test module setup correctly mocks all required dependencies including the EmailService
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check test coverage for notification scenarios
# Find all notification test files
echo "Checking for notification test files:"
fd "notification.*\.spec\.ts" apps/judicial-system/backend/src
# Check for whitelist-related tests
echo -e "\nChecking for whitelist-related test coverage:"
rg -i "whitelist|allowlist" apps/judicial-system/backend/src
# Check environment-specific tests
echo -e "\nChecking for environment-specific test coverage:"
rg "process\.env\.NODE_ENV.*=.*('development'|'staging')" apps/judicial-system/backend/src
Length of output: 9238
Script:
#!/bin/bash
# Let's check the implementation of the notification service and its test coverage
# Check the base notification service implementation
echo "Checking base notification service implementation:"
cat apps/judicial-system/backend/src/app/modules/notification/baseNotification.service.ts
# Check the specific test file for whitelist testing
echo -e "\nChecking whitelist-related test coverage:"
cat apps/judicial-system/backend/src/app/formatters/formatters.spec.ts
# Check if the test file under review uses the base notification service
echo -e "\nChecking the specific test file:"
cat apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/sendIndictmentDeniedNotifications.spec.ts
Length of output: 72293
Script:
#!/bin/bash
# Let's check the implementation of the notification service to understand how email whitelisting is handled
# Check the base notification service implementation
echo "Checking notification service implementation:"
ast-grep --pattern 'class $_ extends BaseNotificationService {
$$$
}'
# Check the test module setup to see how email service is mocked
echo -e "\nChecking test module setup:"
cat apps/judicial-system/backend/src/app/modules/notification/test/createTestingNotificationModule.ts
Length of output: 362453
apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/sendIndictmentReturnedNotifications.spec.ts (2)
11-14: LGTM! Good use of shared test utilities.
The addition of createTestUsers utility promotes consistency across tests and reduces duplication.
33-36: Verify email whitelist implementation for test users
Since this PR implements email whitelisting, we should verify that test users are properly configured for the development/staging environments.
✅ Verification successful
Let me gather more information about the email whitelist implementation and test users.
Let me check the implementation of createTestUsers to understand how test users are configured.
Based on the implementation review, I can now provide the final response:
Test users are properly configured for email whitelisting
The email whitelisting is correctly handled in the codebase:
- Test users are created with
@omnitrix.isdomain emails which aligns with the whitelisting implementation inbaseNotification.service.ts - The service has proper whitelist handling that only runs in non-production environments
- Test emails follow a consistent pattern (
${role}-${uuid}@omnitrix.is) making them easily identifiable for whitelisting
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for whitelist configuration in the codebase
rg -g '!*.spec.ts' "whitelist.*email"
# Check if createTestUsers handles whitelisting
ast-grep --pattern 'function createTestUsers($_) {
$$$
}'
Length of output: 412
Script:
#!/bin/bash
# Check the implementation of email whitelist in baseNotification.service.ts
cat apps/judicial-system/backend/src/app/modules/notification/baseNotification.service.ts
# Search for test user email configuration
rg "createTestUsers" -A 10
Length of output: 74383
Script:
#!/bin/bash
# Check the implementation of createTestUsers
cat apps/judicial-system/backend/src/app/modules/notification/test/createTestingNotificationModule.ts
Length of output: 5411
apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/sendDefendantsNotUpdatedAtCourtNotifications.spec.ts (3)
37-41: LGTM! Improved test setup with centralized user creation
The use of createTestUsers utility improves maintainability and consistency across tests.
Also applies to: 45-45
102-103: LGTM! Consistent use of NotificationType
The transition from CaseNotificationType to NotificationType in the notification check is correct.
5-9: Verify the necessity of both notification type enums
Both CaseNotificationType and NotificationType are imported, but CaseNotificationType seems to be in transition to NotificationType. Consider consolidating to use only NotificationType if possible.
✅ Verification successful
Let me analyze the results and generate a final response.
The search results show that CaseNotificationType is extensively used throughout the codebase, particularly in the notification and case services. Looking at libs/judicial-system/types/src/lib/notification.ts, we can see that NotificationType is actually mapped from CaseNotificationType, suggesting that NotificationType is a new enum that's being introduced as a replacement.
Both notification type enums are currently necessary
The CaseNotificationType enum is still actively used across the codebase, while NotificationType appears to be a new mapping layer. This is likely part of a gradual transition strategy. Keep both imports for now.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if CaseNotificationType is still used elsewhere in the codebase
rg "CaseNotificationType\." --type ts
Length of output: 25296
apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/sendRevokedNotifications.spec.ts (3)
5-8: LGTM: Import changes are well-organized
The addition of NotificationType and createTestUsers imports supports the refactoring of test user data management.
Also applies to: 10-13
87-88: Verify the notification type selection for test scenario
The test uses COURT_DATE as a previous notification type. Let's verify this is the appropriate type for testing revoked notifications behavior.
✅ Verification successful
Let me analyze the usage of COURT_DATE notification type in the codebase to verify if it's appropriate for testing revoked notifications behavior.
Let me check the test file context to understand how the COURT_DATE notification is used in the revoked notifications test.
The COURT_DATE notification type is appropriate for testing revoked notifications
The test file shows that COURT_DATE notification is used as a previous notification state to verify the behavior when a case is revoked. This is correct because:
- The test simulates a real scenario where a court date was previously set (COURT_DATE notification sent)
- Then the case gets revoked, requiring notifications to be sent to all relevant parties (judge, registrar, defender)
- The defender's previous notification status (COURT_DATE) is important to verify they were involved in the case before revocation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check other test files for notification type patterns
rg "NotificationType\.COURT_DATE" -A 5 -B 5
Length of output: 41629
Script:
#!/bin/bash
# Check for revoked notification handling and its relationship with court date notifications
rg "REVOKED" -A 5 -B 5
Length of output: 61691
Script:
#!/bin/bash
# Check the test file content
cat apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/sendRevokedNotifications.spec.ts
Length of output: 4501
28-32: Verify test user creation aligns with email whitelist implementation
The use of createTestUsers improves test maintainability. However, since this PR implements email whitelisting, we should verify that the test users are properly configured for whitelist testing.
✅ Verification successful
Test users are properly configured for email whitelist testing
The createTestUsers utility generates test users with email addresses in the format ${role}-${uuid}@omnitrix.is, which follows a consistent pattern suitable for email whitelist testing. The implementation:
- Creates unique email addresses for each role
- Uses a controlled domain (@omnitrix.is) that can be whitelisted
- Generates consistent test data with all required user properties
This approach aligns with email whitelist testing requirements as it provides predictable email patterns that can be validated against whitelist configurations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if createTestUsers handles whitelist configuration
ast-grep --pattern 'function createTestUsers($_) {
$$$
}'
Length of output: 61
Script:
#!/bin/bash
# Search for createTestUsers function definition with ripgrep
rg -l "createTestUsers" --type ts
# Then get the content of files containing createTestUsers
rg "createTestUsers" -B 5 -A 15 --type ts
Length of output: 124470
Script:
#!/bin/bash
# Check createTestUsers implementation for email whitelist handling
cat apps/judicial-system/backend/src/app/modules/notification/test/createTestingNotificationModule.ts
Length of output: 5411
apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/sendAppealJudgesAssignedNotifications.spec.ts (1)
11-14: LGTM! Clean import organization
The addition of createTestUsers import follows good practices by grouping related imports and introducing a reusable testing utility.
apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/sendAppealFilesUpdatedNotifications.spec.ts (2)
11-14: LGTM! Good modularization of test utilities.
The introduction of centralized test user creation utilities improves code organization and reusability.
60-79: LGTM! Consistent and maintainable user data handling.
The use of centralized test user properties ensures consistency and reduces duplication in the test data setup.
apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/sendIndictmentsWaitingForConfirmationNotifications.spec.ts (1)
7-10: LGTM! Good use of shared test utilities.
The addition of createTestUsers from a shared module improves test maintainability and reduces code duplication.
apps/judicial-system/backend/src/app/modules/notification/test/createTestingNotificationModule.ts (1)
7-7: Verify usage of FormatMessage import
The FormatMessage type appears to be unused in this file. Let's verify if it's needed.
apps/judicial-system/backend/src/app/modules/notification/baseNotification.service.ts (2)
12-12: LGTM: Import statement is correctly structured.
90-94: Verify email delivery in non-production environments.
The whitelist filtering is correctly implemented for non-production environments. However, we should ensure that:
- Test environments have appropriate whitelist configurations
- Existing tests are updated to account for email filtering
Let's verify the test coverage:
✅ Verification successful
Email whitelist implementation is properly configured and tested
The implementation is correctly designed with appropriate safeguards:
- The whitelist is only applied in non-production environments
- There is proper test coverage for the whitelist functionality in
formatters.spec.ts - The whitelist configuration is managed through the notifications messages system
- Failed whitelist matches are properly logged with warning messages
- The system supports both individual email whitelisting and domain whitelisting
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for related test files and their coverage of email sending
rg -t ts "describe.*sendEmail|test.*sendEmail" apps/judicial-system/backend/src/
Length of output: 82
Script:
#!/bin/bash
# Let's try a broader search for test files related to notifications or emails
rg -t ts "describe.*[Nn]otification|test.*email" apps/judicial-system/backend/src/ -l
# Also search for the actual implementation of handleWhitelist to understand its behavior
ast-grep --pattern 'handleWhitelist($_) {
$$$
}'
# Check for whitelist configuration in test or environment files
rg -t ts -t js -t json "whitelist" apps/judicial-system/backend/
Length of output: 7143
Script:
#!/bin/bash
# Let's examine the test coverage for email whitelist functionality
cat apps/judicial-system/backend/src/app/formatters/formatters.spec.ts
# Check the notifications configuration structure
cat apps/judicial-system/backend/src/app/messages/notifications.ts
# Look for environment-specific configurations
rg -t ts -t js -t json "production.*email|email.*production" apps/judicial-system/backend/
Length of output: 114747
apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/sendCaseFilesUpdatedNotifications.spec.ts (1)
13-16: LGTM! Good use of named imports
The addition of these imports improves test organization by leveraging shared test utilities.
apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/defendantNotification/sendDefenderAssignedNotifications.spec.ts (3)
12-15: LGTM! Clean import structure.
The addition of createTestUsers import alongside createTestingNotificationModule improves test data management.
99-100: LGTM! Consistent test data usage.
Using the defender object from createTestUsers for both name and email improves consistency and maintainability of the test data.
166-166: LGTM! Comprehensive test coverage maintained.
The use of defender.email in the unconfirmed defender test case maintains consistency while properly testing the notification blocking logic.
✅ Verification successful
LGTM! The test coverage for unconfirmed defender notification is properly implemented.
The test file shows comprehensive coverage for defender notifications, including:
- Proper test case for unconfirmed defender where no notification is sent
- Verification that
defender.emailis correctly used in test data - Validation that no notification is recorded for unconfirmed defenders
- Additional test coverage for edge cases (defender without email)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify test coverage for other notification scenarios
ast-grep --pattern 'describe.*notification.*\{
$$$
it.*should not send.*\{
$$$
\}
$$$
}'
Length of output: 102
Script:
#!/bin/bash
# Let's try a more specific search for notification test files and their content
fd "notification.*spec.ts" | while read -r file; do
echo "=== $file ==="
rg -A 5 "should not send" "$file" || true
done
Length of output: 23220
Script:
#!/bin/bash
# Search for test cases specifically in the defender notification context
rg -B 2 -A 5 "defender.*notification" "apps/judicial-system/backend/src/app/modules/notification/test/" || true
Length of output: 14300
Script:
#!/bin/bash
# Look for the specific test file and its content
cat "apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/defendantNotification/sendDefenderAssignedNotifications.spec.ts"
Length of output: 6020
apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/sendAppealReceivedByCourtNotifications.spec.ts (2)
Line range hint 13-33: Great improvement in test user management!
The introduction of createTestUsers utility enhances test maintainability and reduces duplication. The centralized approach to creating test users makes the test setup more maintainable and consistent across different test files.
46-46: Verify UUID consistency in COURTS_EMAILS configuration
The hardcoded UUID 4676f08b-aab4-4b4f-a366-697540788088 in the COURTS_EMAILS configuration should be verified against the production configuration to ensure consistency.
✅ Verification successful
UUID in COURTS_EMAILS is correctly used for Court of Appeals
The UUID 4676f08b-aab4-4b4f-a366-697540788088 is the official identifier for the Court of Appeals ("Landsréttur") as defined in the system configuration and database migrations. Its usage in the test file is consistent with the production configuration:
- It's defined as
courtOfAppealsIdinnotification.config.ts - It was added as the institution ID for "Landsréttur" (Court of Appeals) in the database migration
- The UUID is consistently used across multiple test files for Court of Appeals notifications
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other occurrences of this UUID in the codebase
rg -l "4676f08b-aab4-4b4f-a366-697540788088"
# Search for COURTS_EMAILS configuration in other files
rg "COURTS_EMAILS.*=.*{.*}"
Length of output: 957
Script:
#!/bin/bash
# Let's check the content of notification.config.ts to understand the UUID usage
cat apps/judicial-system/backend/src/app/modules/notification/notification.config.ts
# Let's also check the COURTS_EMAILS pattern with proper escaping
rg "COURTS_EMAILS.*=.*\{.*\}"
# Check migrations to understand the context of this UUID
cat apps/judicial-system/backend/migrations/20210830134526-update-institution.js
Length of output: 3861
apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/sendCourtDateNotifications.spec.ts (1)
148-149: Verify the notification type change impact.
The change from CaseNotificationType.READY_FOR_COURT to NotificationType.READY_FOR_COURT seems unrelated to the PR's email whitelisting objective. Please clarify:
- Is this an intentional change?
- How does this relate to the email whitelisting implementation?
apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/sendAppealToCourtOfAppealsNotifications.spec.ts (2)
13-16: LGTM! Good refactoring of test user creation.
The introduction of createTestUsers utility improves code organization and reduces duplication. The array-based role specification makes it clear which user roles are needed for the tests.
Also applies to: 29-35
65-76: Verify email whitelisting implementation.
Based on the PR objectives to implement email whitelisting for development and staging environments, we should verify that these tests will work correctly with the whitelisting mechanism.
Also applies to: 105-202
✅ Verification successful
Let me check the implementation details of the email whitelisting to verify this concern.
Based on the implementation details found, I can now provide a conclusive response about the email whitelisting concern.
Email whitelisting is properly implemented and will work with these tests
The codebase has a robust email whitelisting mechanism that:
- Only applies in non-production environments (
!this.config.production) - Uses both domain whitelisting and specific email whitelisting
- Is configured through translation messages for flexibility
- Logs warnings when emails are filtered out
- Is implemented in the base notification service that all notification flows inherit from
The test file in question doesn't need modifications because:
- The whitelisting is handled automatically by the base service
- The whitelisting only runs in non-production environments
- The test environment will bypass the whitelisting as it's considered a production environment in the test context
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if email whitelisting is implemented in the notification system
# Look for email whitelist configuration
echo "Searching for email whitelist configuration..."
rg -l "emailWhitelist|whitelistEmails" --type ts
# Look for environment-specific email handling
echo "Searching for environment-specific email handling..."
rg -l "process\.env\.(NODE_ENV|ENVIRONMENT)" --type ts -A 5 | xargs rg "email" -l
Length of output: 977
Script:
#!/bin/bash
# Check the email whitelisting implementation in notification service
echo "Checking base notification service implementation..."
cat apps/judicial-system/backend/src/app/modules/notification/baseNotification.service.ts
echo "\nChecking notification formatters..."
cat apps/judicial-system/backend/src/app/formatters/formatters.ts
echo "\nChecking notification configuration..."
cat apps/judicial-system/backend/src/app/messages/notifications.ts
Length of output: 80682
apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/sendAppealCompletedNotifications.spec.ts (2)
Line range hint 15-40: LGTM! Good improvement in test data management.
The introduction of createTestUsers utility enhances maintainability by centralizing test user creation and following DRY principles.
76-81: LGTM! Well-structured test data setup.
The use of test user properties improves readability and type safety while maintaining a clear separation of concerns.
apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/sendAppealStatementNotifications.spec.ts (2)
71-81: LGTM! Clean refactor of case data structure.
The updated case data structure using the new user objects improves consistency and reduces duplication.
122-122: LGTM! Verify test coverage completeness.
The email recipient updates are consistent across all scenarios and properly utilize the new user objects.
Let's verify that all notification scenarios are covered:
Also applies to: 152-152, 179-179, 204-204, 226-226, 245-245
✅ Verification successful
Let me analyze the test coverage by checking the implementation of the appeal statement notification test file.
Test coverage for appeal statement notifications is comprehensive
The test file thoroughly covers all notification scenarios for appeal statements:
- Prosecutor sending notification with/without appeal case number
- Defender sending notification with/without appeal case number
- Cases with/without defender national ID
- Proper recipient lists for each scenario (prosecutor, defender, court officials)
- Correct email content including subject lines and links
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any missing notification scenarios in the test file
# Look for all CaseNotificationType usages in the codebase
echo "Checking for all possible notification types:"
rg "CaseNotificationType\." -A 1
# Look for other notification test files to compare coverage
echo "Checking for other notification test files:"
fd "notification.*\.spec\.ts$"
Length of output: 48026
Script:
#!/bin/bash
# Check the implementation of appeal statement notifications test
echo "Checking appeal statement notifications test implementation:"
cat "apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/sendAppealStatementNotifications.spec.ts"
Length of output: 9187
apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/sendAdvocateAssignedNotifications.spec.ts (5)
17-20: LGTM! Good improvement in test data management.
The introduction of createTestUsers utility and its usage improves test maintainability by centralizing test user creation. This follows testing best practices by making the test data setup more consistent and reusable.
Also applies to: 42-47
56-57: LGTM! Clean test module initialization.
The test module initialization is well-structured and properly integrates with the new test utilities.
210-211: LGTM! Good test coverage for restriction cases.
The update to use dynamic test user data maintains clear test scenarios for both cases with and without national ID, properly validating the different email templates.
Also applies to: 252-253
293-294: LGTM! Important negative test case maintained.
The update to use dynamic test user data preserves the important test case verifying that no emails are sent for investigation cases.
93-94: LGTM! Verify email templates remain accurate.
The update to use dynamic test user data improves maintainability while preserving the test scenarios for both lawyer and legal rights protector roles.
Also applies to: 140-141
✅ Verification successful
Let me gather more information about the email templates and their usage.
Email templates are consistent and correctly used
The email templates for advocate assignments are properly defined in notifications.ts and correctly used in the test file. The templates handle both lawyer and legal rights protector roles with appropriate text variations:
- For lawyers: "lögmann einkaréttarkröfuhafa"
- For legal rights protectors: "réttargæslumann einkaréttarkröfuhafa"
The dynamic test user data maintains consistency with these templates while improving test maintainability.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify email templates consistency across test files
# Search for similar email templates in other test files
rg -A 2 "Skráning í máli" apps/judicial-system/backend/src/app/modules/notification/test/
Length of output: 1710
Script:
#!/bin/bash
# Search for email template definitions and usage
ast-grep --pattern 'sendAdvocateAssignedNotifications($$$)'
# Search for any other files that might be using these templates
rg -l "lögmann einkaréttarkröfuhafa|réttargæslumann einkaréttarkröfuhafa" apps/judicial-system/backend/src/
Length of output: 370
Script:
#!/bin/bash
# Check the actual email template definitions in notifications.ts
cat apps/judicial-system/backend/src/app/messages/notifications.ts
Length of output: 51152
apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/sendAppealWithdrawnNotifications.spec.ts (2)
8-8: LGTM: Clean import organization
The addition of NotificationType and createTestUsers utility imports are well-organized and follow TypeScript best practices.
Also applies to: 13-16
34-52: Great refactor: Improved test data management
The use of createTestUsers utility is a significant improvement that:
- Centralizes user creation logic
- Ensures consistent test data
- Reduces code duplication
- Makes tests more maintainable
apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/sendRulingNotifications.spec.ts (3)
19-22: LGTM! Good improvement in test data management
Using createTestUsers helper improves maintainability and consistency of test data across the test suite.
Also applies to: 49-49
180-180: LGTM! Consistent test expectations
The email service expectations correctly use the test user properties, maintaining consistency with the new test data management approach.
57-58: Verify email domain whitelisting configuration
The test is using omnitrix.is domain for prison emails. Ensure this domain is properly whitelisted in the development and staging environments as per PR objectives.
✅ Verification successful
Domain whitelisting configuration is properly implemented
The omnitrix.is domain is correctly configured as the default domain across the application:
- Default sender email:
ben10@omnitrix.is - Default reply-to email:
ben10@omnitrix.is - Default court robot email:
ben10@omnitrix.is - Default support email:
ben10@omnitrix.is
The test's use of prisonEmail@omnitrix.is aligns with the application-wide email domain configuration, and the environment variables allow for overriding these values in different environments.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for email domain whitelist configuration
# Expected: Find whitelist configuration files that should include omnitrix.is
# Search for whitelist configuration files
fd -e json -e yaml -e env -e ts -e js | xargs rg -l "omnitrix\.is"
Length of output: 543
Script:
#!/bin/bash
# Check the content of relevant configuration files to verify domain whitelisting
echo "=== Checking notification.config.ts ==="
rg -A 5 -B 5 "omnitrix\.is" "apps/judicial-system/backend/src/app/modules/notification/notification.config.ts"
echo -e "\n=== Checking court.config.ts ==="
rg -A 5 -B 5 "omnitrix\.is" "apps/judicial-system/backend/src/app/modules/court/court.config.ts"
echo -e "\n=== Checking next.config.js ==="
rg -A 5 -B 5 "omnitrix\.is" "apps/judicial-system/web/next.config.js"
Length of output: 2135
apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/sendReadyForCourtNotifications.spec.ts (4)
18-18: LGTM: Improved test setup with centralized user management
The addition of NotificationType and test utilities enhances code organization and maintainability by centralizing test user creation.
Also applies to: 23-26
53-57: LGTM: Enhanced test data management
The use of createTestUsers utility improves test maintainability by:
- Centralizing test user creation
- Making it easier to update user properties consistently
- Reducing duplication of test data
Also applies to: 65-66, 68-68, 71-73
89-89: LGTM: Proper test environment configuration
The environment setup correctly mirrors the production configuration structure for court mobile numbers.
131-131: LGTM: Consistent recipient handling in test assertions
The test assertions properly validate:
- Email notifications to whitelisted recipients
- SMS notifications to whitelisted court numbers
- Proper handling of defender notifications based on sharing settings
Also applies to: 141-142, 187-187, 197-197, 205-205, 259-259, 342-342, 352-354, 398-398, 408-410
...s/notification/test/internalNotificationController/sendIndictmentDeniedNotifications.spec.ts
Show resolved
Hide resolved
...st/internalNotificationController/sendIndictmentsWaitingForConfirmationNotifications.spec.ts
Show resolved
Hide resolved
.../modules/notification/test/internalNotificationController/sendCourtDateNotifications.spec.ts
Show resolved
Hide resolved
.../modules/notification/test/internalNotificationController/sendCourtDateNotifications.spec.ts
Show resolved
Hide resolved
...fication/test/internalNotificationController/sendAppealToCourtOfAppealsNotifications.spec.ts
Show resolved
Hide resolved
...es/notification/test/internalNotificationController/sendAppealWithdrawnNotifications.spec.ts
Show resolved
Hide resolved
...ules/notification/test/internalNotificationController/sendReadyForCourtNotifications.spec.ts
Show resolved
Hide resolved
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16907 +/- ##
==========================================
+ Coverage 36.43% 36.47% +0.04%
==========================================
Files 6904 6903 -1
Lines 144701 144549 -152
Branches 41345 41262 -83
==========================================
+ Hits 52723 52728 +5
+ Misses 91978 91821 -157
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 15 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Datadog ReportAll test runs ✅ 2 Total Test Services: 0 Failed, 2 Passed Test Services
|
* chore(j-s): Whitelist emails outside prod * fix(j-s): Tests after whitelisting * fix(j-s): more tests * fix(j-s): And more tests fixed * Update sendAdvocateAssignedNotifications.spec.ts --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
Email á dev fari ekki á raunverulega verjendur
What
Use whitelisting on dev and staging so emails don't get sent out to people who shouldn't receive them
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores