chore(j-s): Civil claimant spokesperson assigned notifications #16750
Conversation
…/civil-claimant-assigned-notifications
WalkthroughThe pull request primarily involves the removal of the Changes
Possibly related PRs
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
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16750 +/- ##
==========================================
+ Coverage 36.43% 36.58% +0.14%
==========================================
Files 6855 6862 +7
Lines 143681 144211 +530
Branches 41019 41277 +258
==========================================
+ Hits 52356 52753 +397
- Misses 91325 91458 +133
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 44 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Datadog ReportAll test runs ✅ 9 Total Test Services: 0 Failed, 9 Passed Test Services
🔻 Code Coverage Decreases vs Default Branch (1)
|
…/civil-claimant-assigned-notifications
…/civil-claimant-assigned-notifications
unakb
changed the title
unakb
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Outside diff range and nitpick comments (42)
apps/judicial-system/backend/src/app/modules/defendant/guards/civilClaimaint.decorator.ts (2)
1-7: Rename file to fix typo in filenameThe file name contains a typo: "civilClaimaint" should be "civilClaimant". This should be corrected to maintain consistency with the model name and prevent confusion.
5-7: Add type safety and null checksThe decorator implementation could benefit from improved type safety and null checking:
- Add type for the GraphQL context
- Add null check for the civilClaimant property
Consider applying this improvement:
-export const CurrentCivilClaimant = createParamDecorator( - (data, { args: [_1, { req }] }): CivilClaimant => req.civilClaimant, -) +import { ExecutionContext } from '@nestjs/common' + +export const CurrentCivilClaimant = createParamDecorator( + (data: unknown, ctx: ExecutionContext): CivilClaimant => { + const [, { req }] = ctx.getArgs() + if (!req.civilClaimant) { + throw new Error('Civil claimant not found in request') + } + return req.civilClaimant + }, +)apps/judicial-system/backend/src/app/modules/notification/dto/civilClaimantNotification.dto.ts (1)
7-12: Consider enhancing API documentation and type safety.The implementation is solid with proper validation and type safety. However, there are two suggestions for improvement:
- Add class-level API documentation to describe the DTO's purpose
- Consider initializing the property instead of using non-null assertion
Here's how you could enhance it:
+@ApiProperty({ description: 'DTO for civil claimant notifications' }) export class CivilClaimantNotificationDto { @IsNotEmpty() @IsEnum(CivilClaimantNotificationType) @ApiProperty({ enum: CivilClaimantNotificationType }) - readonly type!: CivilClaimantNotificationType + readonly type: CivilClaimantNotificationType = CivilClaimantNotificationType.ASSIGNED }apps/judicial-system/backend/src/app/modules/defendant/index.ts (1)
1-9: Consider organizing exports with commentsConsider grouping related exports with comments for better code organization:
+// Defendant-related exports export { Defendant } from './models/defendant.model' export { DefendantService } from './defendant.service' export { DefendantExistsGuard } from './guards/defendantExists.guard' export { CurrentDefendant } from './guards/defendant.decorator' +// Civil Claimant-related exports export { CivilClaimant } from './models/civilClaimant.model' export { CivilClaimantService } from './civilClaimant.service' export { CivilClaimantExistsGuard } from './guards/civilClaimantExists.guard' export { CurrentCivilClaimant } from './guards/civilClaimant.decorator'apps/judicial-system/backend/src/app/modules/defendant/test/civilClaimantController/deleteRolesRules.spec.ts (2)
8-9: Consider using proper TypeScript types instead ofany.While the eslint disable comment acknowledges the use of
any, it would be better to define proper types for the rules array.- // eslint-disable-next-line @typescript-eslint/no-explicit-any - let rules: any[] + let rules: Array<(target: any, propertyKey: string) => void>
20-25: Consider strengthening the test assertions.While the current test verifies the presence and number of rules, it could be enhanced to ensure the rules are in the correct order and that no additional unexpected rules are present.
it('should give permission to roles', () => { expect(rules).toHaveLength(expectedRules.length) - expectedRules.forEach((expectedRule) => - expect(rules).toContain(expectedRule), - ) + expect(rules).toEqual(expect.arrayContaining(expectedRules)) + // Ensure no unexpected rules + expect(rules.every(rule => expectedRules.includes(rule))).toBe(true) })apps/judicial-system/backend/src/app/modules/defendant/test/civilClaimantController/createGuards.spec.ts (2)
6-15: Consider adding test cases for invalid metadata scenarios.While the current setup is good, the test suite could be more robust by handling edge cases where metadata might be undefined or malformed.
Consider adding:
it('should handle missing metadata gracefully', () => { const invalidMetadata = Reflect.getMetadata( '__guards__', CivilClaimantController.prototype.nonexistentMethod, ) expect(invalidMetadata).toBeUndefined() })
1-25: Consider adding more comprehensive test coverage.While the basic guard verification is good, consider adding tests for:
- Guard behavior when applied to other controller methods
- Integration with actual request context
Would you like me to provide examples of additional test cases?
apps/judicial-system/backend/src/app/modules/defendant/test/civilClaimantController/deleteGuards.spec.ts (1)
17-24: Consider enhancing test coverage.While the current test verifies guard presence and order, consider adding negative test cases to ensure the guards actually prevent unauthorized access.
Add tests like:
it('should prevent deletion when case does not exist', () => { const caseExistsGuard = new guards[0](); // Mock canActivate context where case doesn't exist expect(await caseExistsGuard.canActivate(context)).toBeFalsy(); }); it('should prevent deletion without write permission', () => { const writeGuard = new guards[1](); // Mock canActivate context without write permission expect(await writeGuard.canActivate(context)).toBeFalsy(); });apps/judicial-system/backend/src/app/modules/defendant/test/civilClaimantController/updateGuards.spec.ts (1)
17-24: Consider adding edge cases to strengthen the test suite.While the current test effectively verifies guard presence and order, consider adding tests for:
- Error cases when guards are missing
- Behavior when guards are applied in the wrong order
Here's a suggested additional test:
it('should fail if guards are missing', () => { // Mock removing one guard jest.spyOn(Reflect, 'getMetadata').mockReturnValueOnce([CaseExistsGuard]) expect(guards.length).not.toBe(expectedGuards.length) })apps/judicial-system/backend/src/app/modules/defendant/test/civilClaimantController/createRolesRules.spec.ts (2)
11-12: Consider using a more specific type instead ofany.Replace the
any[]type with a more specific type to improve type safety. You could define an interface or type for the role rules.- // eslint-disable-next-line @typescript-eslint/no-explicit-any - let rules: any[] + type RoleRule = typeof prosecutorRule + let rules: RoleRule[]
29-34: Consider enhancing test failure messages.While the test is functionally correct, it could provide more specific feedback when it fails. Consider adding custom error messages to identify which specific roles are missing.
it('should give permission to roles', () => { expect(rules).toHaveLength(expectedRules.length) - expectedRules.forEach((expectedRule) => - expect(rules).toContain(expectedRule), - ) + expectedRules.forEach((expectedRule) => { + expect(rules).toContain( + expectedRule, + `Missing permission for role: ${expectedRule.name}`, + ) + }) })apps/judicial-system/backend/src/app/modules/defendant/test/civilClaimantController/updateRolesRules.spec.ts (1)
11-12: Consider removing eslint-disable comment by properly typing the rules array.Instead of using
any[]with an eslint-disable comment, consider defining a proper type for the rules array to maintain type safety.- // eslint-disable-next-line @typescript-eslint/no-explicit-any - let rules: any[] + let rules: Array<{ + role: string; + // Add other expected properties based on the actual rule structure + }>apps/judicial-system/backend/src/app/modules/notification/civilClaimantNotification.strings.ts (1)
4-16: Enhance documentation with JSDoc comments.Add JSDoc comments to document the message definitions, including:
- Purpose of each message
- Required placeholders and their expected values
- HTML content handling requirements
/** * Messages for civil claimant spokesperson notifications. * @property civilClaimantSpokespersonAssignedSubject - Subject line for assignment notification * @property civilClaimantSpokespersonAssignedBody - Body text for assignment notification * * Required placeholders: * - courtName: Name of the court * - spokespersonIsLawyer: Boolean indicating if spokesperson is a lawyer * - courtCaseNumber: Case identifier * - spokespersonHasAccessToRVG: Boolean for RVG access * * Note: Contains HTML content that must be sanitized before rendering */ export const strings = { // ... existing code ... }apps/judicial-system/backend/src/app/modules/defendant/guards/civilClaimantExists.guard.ts (1)
38-38: Document request object modification.Add a comment explaining that the guard enriches the request object with the civil claimant for downstream handlers.
+ // Attach validated civil claimant to request for use in route handlers request.civilClaimant = civilClaimantapps/judicial-system/backend/src/app/modules/notification/notification.module.ts (1)
21-21: Consider grouping related notification servicesWhile the implementation looks good, consider organizing the notification services into feature-specific sub-modules for better maintainability as the number of notification types grows.
You could create a structure like this:
// notification-services/index.ts export * from './case/case-notification.service'; export * from './civil-claimant/civil-claimant-notification.service'; // ... other services // notification.module.ts import * as NotificationServices from './notification-services'; @Module({ // ... providers: [ ...Object.values(NotificationServices), NotificationService, NotificationDispatchService, ], })Also applies to: 48-52
apps/judicial-system/backend/src/app/modules/defendant/test/civilClaimantController/delete.spec.ts (4)
8-17: Fix typo in parameter name and enhance type safety.The type definitions are well-structured, but there's a typo in the parameter name 'civilClaimaintId' and the types could be more specific.
type GivenWhenThen = ( caseId?: string, - civilClaimaintId?: string, + civilClaimantId?: string, ) => Promise<Then>Consider making the interface more type-safe:
interface Then { result: DeleteCivilClaimantResponse | null error: Error | null }
25-48: LGTM! Well-structured test setup with proper error handling.The test setup follows best practices with proper mocking and error handling. The givenWhenThen pattern is well implemented.
Consider using a more specific error type instead of the generic Error class for better type safety:
catch (error) { then.error = error instanceof Error ? error : new Error('Unknown error'); }
50-65: Consider adding more test cases for comprehensive coverage.While the happy path is well tested, consider adding these scenarios:
- When destroy returns 0 (no rows affected)
- When destroy returns multiple rows affected
Example additional test:
describe('when no civil claimant found', () => { beforeEach(async () => { const mockDestroy = mockCivilClaimantModel.destroy as jest.Mock mockDestroy.mockResolvedValue(0) then = await givenWhenThen(caseId, civilClaimantId) }) it('should return deleted: false', () => { expect(then.result).toEqual({ deleted: false }) }) })
67-78: Enhance error testing with specific error cases.The current error testing is basic. Consider adding tests for specific error scenarios that could occur in production:
- Database connection errors
- Foreign key constraint violations
- Permission errors
Example:
describe('when database constraint violation occurs', () => { beforeEach(async () => { const mockDestroy = mockCivilClaimantModel.destroy as jest.Mock mockDestroy.mockRejectedValue(new Error('foreign key constraint violation')) then = await givenWhenThen(caseId, civilClaimantId) }) it('should throw appropriate error', () => { expect(then.error).toBeInstanceOf(Error) expect(then.error.message).toContain('constraint violation') }) })apps/judicial-system/backend/src/app/modules/defendant/test/civilClaimantController/create.spec.ts (2)
8-11: Consider making Then interface fields optionalThe interface fields
resultanderrorare mutually exclusive - a response will either have a result or an error, but not both.interface Then { - result: CivilClaimant - error: Error + result?: CivilClaimant + error?: Error }
60-62: Enhance test coverage with comprehensive assertionsThe current test only verifies object equality. Consider adding specific assertions for individual fields to make test failures more descriptive.
it('should return the created civil claimant', () => { - expect(then.result).toEqual(createdCivilClaimant) + expect(then.result).toBeDefined() + expect(then.result?.id).toBe(civilClaimantId) + expect(then.result?.caseId).toBe(caseId) })libs/judicial-system/types/src/lib/notification.ts (1)
Line range hint
33-61: Consider adding JSDoc comments for the notification types.To improve maintainability and developer experience, consider adding documentation that explains:
- The purpose of each notification type
- When each notification is triggered
- Who receives these notifications
Example:
+/** + * Notification types specific to civil claimant actions. + * These notifications are sent to relevant parties in civil cases. + */ export enum CivilClaimantNotificationType { + /** Sent when a spokesperson is officially assigned to a civil case */ SPOKESPERSON_ASSIGNED = 'SPOKESPERSON_ASSIGNED', }apps/judicial-system/backend/src/app/modules/defendant/civilClaimant.service.ts (1)
Line range hint
53-78: Consider adding transaction management.The update method performs two operations (database update and notification) that should maintain consistency. Consider wrapping both operations in a transaction to ensure atomicity.
Example approach:
async update( caseId: string, civilClaimantId: string, update: UpdateCivilClaimantDto, ): Promise<CivilClaimant> { return await this.civilClaimantModel.sequelize.transaction(async (t) => { const [numberOfAffectedRows, civilClaimants] = await this.civilClaimantModel.update(update, { where: { id: civilClaimantId, caseId: caseId }, returning: true, transaction: t, }); // ... existing validation logic ... await this.sendUpdateCivilClaimantMessages(update, civilClaimants[0]); return civilClaimants[0]; }); }apps/judicial-system/backend/src/app/modules/defendant/test/createTestingDefendantModule.ts (1)
99-109: Consider using type parameters for better type safety.The resolution and exports for civil claimant components are correctly implemented. However, consider using explicit type parameters with
resolvefor better type safety:- const civilClaimantModel = await defendantModule.resolve< - typeof CivilClaimant - >(getModelToken(CivilClaimant)) + const civilClaimantModel = await defendantModule.resolve<typeof CivilClaimant>( + getModelToken(CivilClaimant), + { strict: true } + )Also applies to: 120-122
apps/judicial-system/backend/src/app/modules/defendant/test/civilClaimantController/update.spec.ts (3)
11-21: Consider strengthening the TypeScript types.The
Theninterface andGivenWhenThentype could be improved to better handle the mutually exclusive nature of result and error.Consider this alternative implementation:
-interface Then { - result: CivilClaimant - error: Error -} +type Then = { + result: CivilClaimant + error: undefined +} | { + result: undefined + error: Error +}
48-54: Improve clarity of result/error assignments.The current assignments within the promise chain are flagged by static analysis as potentially confusing.
Consider this clearer approach:
- await civilClaimantController - .update(caseId, civilClaimantId, updateData) - .then((result) => (then.result = result)) - .catch((error) => (then.error = error)) + try { + then.result = await civilClaimantController.update( + caseId, + civilClaimantId, + updateData + ) + } catch (error) { + then.error = error as Error + }🧰 Tools
🪛 Biome
[error] 52-52: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 53-53: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
125-139: Consider testing specific error scenarios.While the basic error handling is covered, consider adding test cases for specific error types that might occur during updates (e.g., validation errors, not found errors).
Example test case structure:
describe('when civil claimant not found', () => { beforeEach(async () => { const mockUpdate = mockCivilClaimantModel.update as jest.Mock mockUpdate.mockRejectedValue(new Error('Civil claimant not found')) // ... test implementation }) it('should throw a specific error type', () => { expect(then.error).toBeInstanceOf(NotFoundException) }) })apps/judicial-system/backend/src/app/modules/notification/test/createTestingNotificationModule.ts (1)
Line range hint
134-141: Consider exposing CivilClaimantNotificationService in the test context.The test context object doesn't expose the
CivilClaimantNotificationService, which might be needed for testing civil claimant notification scenarios.const context = { userService: notificationModule.get(UserService), internalCaseService: notificationModule.get(InternalCaseService), + civilClaimantNotificationService: notificationModule.get(CivilClaimantNotificationService), messageService: notificationModule.get(MessageService), // ... other services }apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/civilClaimantNotification/sendSpokespersonAssignedNotifications.spec.ts (3)
23-26: Consider enhancing type safety for the Then interface.The
Theninterface could be more specific about the expected shape ofresultand better handle the optional nature of error cases.Consider this improvement:
interface Then { - result: DeliverResponse - error: Error + result: DeliverResponse | null + error: Error | null }
90-166: Consider adding more test scenarios and improving assertions.While the current test cases cover the basic scenarios, consider adding:
- Edge cases for invalid email addresses
- Cases where spokesperson details are incomplete
- More specific assertions for email content
Example additional test scenario:
describe.each([ { isSpokespersonConfirmed: true, shouldSendEmail: true, spokespersonEmail: 'ben10@omnitrix.is' }, { isSpokespersonConfirmed: true, shouldSendEmail: false, spokespersonEmail: '' // Invalid email }, // ... existing scenarios ])Also, consider adding more specific assertions for the email content:
expect(mockEmailService.sendEmail).toBeCalledWith( expect.objectContaining({ html: expect.stringMatching(/^<!DOCTYPE html>/), text: expect.not.stringContaining('<html>'), }) )
144-148: Consider implementing proper internationalization.The email subject and content contain hardcoded Icelandic strings. Consider using a proper i18n solution for better maintainability and potential future language support.
Consider:
- Extracting these strings to a translation file
- Using a translation service/helper
- Making the language configurable based on user preferences
apps/judicial-system/backend/src/app/modules/notification/internalNotification.controller.ts (2)
142-145: Enhance API description for clarity.The current description is quite generic. Consider adding more details about when and why these notifications are sent.
- description: 'Sends civil claimant related notifications', + description: 'Sends notifications to civil claimant spokespersons when they are officially assigned to indictment cases',
136-162: Consider type-safe route construction.While the current implementation follows the established pattern, consider using a type-safe approach for route construction to prevent potential runtime errors from typos in message endpoints.
Example approach:
const ROUTES = { CIVIL_CLAIMANT_NOTIFICATION: (caseId: string, civilClaimantId: string) => `case/${caseId}/${messageEndpoint[MessageType.CIVIL_CLAIMANT_NOTIFICATION]}/${civilClaimantId}` } as const; @Post(ROUTES.CIVIL_CLAIMANT_NOTIFICATION(':caseId', ':civilClaimantId'))apps/judicial-system/backend/src/app/modules/notification/caseNotification.service.ts (6)
Line range hint
1508-1519: Consider extracting session arrangement validation to a separate method.The session arrangement validation logic is duplicated across multiple methods. Extract it to a reusable method to improve maintainability and reduce code duplication.
+private isDefenderIncludedInSessionArrangements(sessionArrangements?: SessionArrangements): boolean { + return Boolean(sessionArrangements && [ + SessionArrangements.ALL_PRESENT, + SessionArrangements.ALL_PRESENT_SPOKESPERSON, + ].includes(sessionArrangements)) +} private shouldSendAdvocateAssignedNotification( theCase: Case, advocateEmail?: string, ): boolean { if (!advocateEmail) { return false } if (isInvestigationCase(theCase.type)) { - const isDefenderIncludedInSessionArrangements = - theCase.sessionArrangements && - [ - SessionArrangements.ALL_PRESENT, - SessionArrangements.ALL_PRESENT_SPOKESPERSON, - ].includes(theCase.sessionArrangements) - - if (!isDefenderIncludedInSessionArrangements) { + if (!this.isDefenderIncludedInSessionArrangements(theCase.sessionArrangements)) { return false } }
Line range hint
1543-1561: Add error handling for failed notification attempts.The advocate assigned notification logic should handle potential failures gracefully and provide appropriate error logging.
private async sendAdvocateAssignedNotifications( theCase: Case, ): Promise<DeliverResponse> { const promises: Promise<Recipient>[] = [] if (DateLog.arraignmentDate(theCase.dateLogs)?.date) { const shouldSend = this.shouldSendAdvocateAssignedNotification( theCase, theCase.defenderEmail, ) if (shouldSend) { + try { promises.push(this.sendCourtDateEmailNotificationToDefender(theCase)) + } catch (error) { + this.logger.error('Failed to send advocate assigned notification', { + caseId: theCase.id, + error, + }) + } } }
Line range hint
1-50: Consider implementing rate limiting for email notifications.The service sends multiple notifications but lacks rate limiting. This could potentially lead to email service throttling or blacklisting.
Consider implementing a rate limiter using a token bucket or similar algorithm to prevent sending too many notifications in a short period. This could be done by:
- Adding a rate limiter service
- Implementing backoff strategies for failed notifications
- Batching notifications where appropriate
Line range hint
1508-1561: Add unit tests for notification conditions.The notification logic contains complex conditions but lacks comprehensive test coverage.
Would you like me to help create unit tests for:
- Session arrangement validation
- Notification eligibility checks
- Error handling scenarios
Line range hint
1-2500: Consider implementing notification templates using a template engine.The service uses string interpolation for email templates, which can become hard to maintain as templates grow more complex.
Consider using a template engine like Handlebars or EJS to:
- Separate template logic from business logic
- Make templates easier to maintain and update
- Support internationalization more effectively
Line range hint
1-2500: Add monitoring and metrics for notification delivery.The service would benefit from monitoring to track notification success rates and identify potential issues.
Consider adding:
- Prometheus metrics for notification success/failure rates
- Tracking of notification delivery times
- Alerts for high failure rates
- Dashboard for notification system health
apps/judicial-system/backend/src/app/modules/notification/civilClaimantNotification.service.ts (2)
59-59: Use explicit Boolean conversion for clarityConsider using
Boolean()instead of!!for converting values to boolean to enhance readability.Apply this diff to improve clarity:
- const spokespersonHasAccessToRVG = !!civilClaimant.spokespersonNationalId + const spokespersonHasAccessToRVG = Boolean(civilClaimant.spokespersonNationalId)
171-173: Enhance error logging with additional contextIncluding more context in the error log can aid in debugging issues by providing relevant information.
You can modify the error logging statement to include the notification type and case ID:
- this.logger.error('Failed to send notification', error) + this.logger.error(`Failed to send notification of type ${type} for case ${theCase.id}`, { error })
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (28)
-
apps/judicial-system/backend/src/app/formatters/formatters.ts(0 hunks) -
apps/judicial-system/backend/src/app/formatters/index.ts(0 hunks) -
apps/judicial-system/backend/src/app/messages/notifications.ts(0 hunks) -
apps/judicial-system/backend/src/app/modules/defendant/civilClaimant.service.ts(4 hunks) -
apps/judicial-system/backend/src/app/modules/defendant/guards/civilClaimaint.decorator.ts(1 hunks) -
apps/judicial-system/backend/src/app/modules/defendant/guards/civilClaimantExists.guard.ts(1 hunks) -
apps/judicial-system/backend/src/app/modules/defendant/index.ts(1 hunks) -
apps/judicial-system/backend/src/app/modules/defendant/test/civilClaimantController/create.spec.ts(1 hunks) -
apps/judicial-system/backend/src/app/modules/defendant/test/civilClaimantController/createGuards.spec.ts(1 hunks) -
apps/judicial-system/backend/src/app/modules/defendant/test/civilClaimantController/createRolesRules.spec.ts(1 hunks) -
apps/judicial-system/backend/src/app/modules/defendant/test/civilClaimantController/delete.spec.ts(1 hunks) -
apps/judicial-system/backend/src/app/modules/defendant/test/civilClaimantController/deleteGuards.spec.ts(1 hunks) -
apps/judicial-system/backend/src/app/modules/defendant/test/civilClaimantController/deleteRolesRules.spec.ts(1 hunks) -
apps/judicial-system/backend/src/app/modules/defendant/test/civilClaimantController/update.spec.ts(1 hunks) -
apps/judicial-system/backend/src/app/modules/defendant/test/civilClaimantController/updateGuards.spec.ts(1 hunks) -
apps/judicial-system/backend/src/app/modules/defendant/test/civilClaimantController/updateRolesRules.spec.ts(1 hunks) -
apps/judicial-system/backend/src/app/modules/defendant/test/createTestingDefendantModule.ts(5 hunks) -
apps/judicial-system/backend/src/app/modules/notification/caseNotification.service.ts(3 hunks) -
apps/judicial-system/backend/src/app/modules/notification/civilClaimantNotification.service.ts(1 hunks) -
apps/judicial-system/backend/src/app/modules/notification/civilClaimantNotification.strings.ts(1 hunks) -
apps/judicial-system/backend/src/app/modules/notification/dto/civilClaimantNotification.dto.ts(1 hunks) -
apps/judicial-system/backend/src/app/modules/notification/internalNotification.controller.ts(3 hunks) -
apps/judicial-system/backend/src/app/modules/notification/notification.module.ts(2 hunks) -
apps/judicial-system/backend/src/app/modules/notification/test/createTestingNotificationModule.ts(2 hunks) -
apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/civilClaimantNotification/sendSpokespersonAssignedNotifications.spec.ts(1 hunks) -
libs/judicial-system/message/src/lib/message.ts(2 hunks) -
libs/judicial-system/types/src/index.ts(1 hunks) -
libs/judicial-system/types/src/lib/notification.ts(2 hunks)
💤 Files with no reviewable changes (3)
- apps/judicial-system/backend/src/app/formatters/formatters.ts
- apps/judicial-system/backend/src/app/formatters/index.ts
- apps/judicial-system/backend/src/app/messages/notifications.ts
🧰 Additional context used
📓 Path-based instructions (25)
apps/judicial-system/backend/src/app/modules/defendant/civilClaimant.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/defendant/guards/civilClaimaint.decorator.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/defendant/guards/civilClaimantExists.guard.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/defendant/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/modules/defendant/test/civilClaimantController/create.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/defendant/test/civilClaimantController/createGuards.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/defendant/test/civilClaimantController/createRolesRules.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/defendant/test/civilClaimantController/delete.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/defendant/test/civilClaimantController/deleteGuards.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/defendant/test/civilClaimantController/deleteRolesRules.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/defendant/test/civilClaimantController/update.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/defendant/test/civilClaimantController/updateGuards.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/defendant/test/civilClaimantController/updateRolesRules.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/defendant/test/createTestingDefendantModule.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/caseNotification.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/civilClaimantNotification.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/civilClaimantNotification.strings.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/dto/civilClaimantNotification.dto.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/internalNotification.controller.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.module.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/civilClaimantNotification/sendSpokespersonAssignedNotifications.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."
libs/judicial-system/message/src/lib/message.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/judicial-system/types/src/index.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
libs/judicial-system/types/src/lib/notification.ts (1)
Pattern libs/**/*: "Confirm that the code adheres to the following:
- Reusability of components and hooks across different NextJS apps.
- TypeScript usage for defining props and exporting types.
- Effective tree-shaking and bundling practices."
📓 Learnings (12)
apps/judicial-system/backend/src/app/modules/defendant/guards/civilClaimantExists.guard.ts (1)
Learnt from: gudjong
PR: island-is/island.is#16389
File: apps/judicial-system/backend/src/app/modules/defendant/civilClaimant.service.ts:75-95
Timestamp: 2024-10-17T11:45:23.482Z
Learning: The `created` field exists in the `CivilClaimant` model in `civilClaimant.model.ts`.
apps/judicial-system/backend/src/app/modules/defendant/index.ts (2)
Learnt from: oddsson
PR: island-is/island.is#16059
File: apps/judicial-system/backend/src/app/modules/defendant/defendant.module.ts:28-28
Timestamp: 2024-09-18T14:32:59.502Z
Learning: Services should only be exported when they are needed by other modules.
Learnt from: oddsson
PR: island-is/island.is#16059
File: apps/judicial-system/backend/src/app/modules/defendant/defendant.module.ts:28-28
Timestamp: 2024-10-08T15:39:04.351Z
Learning: Services should only be exported when they are needed by other modules.
apps/judicial-system/backend/src/app/modules/defendant/test/civilClaimantController/create.spec.ts (1)
Learnt from: gudjong
PR: island-is/island.is#16389
File: apps/judicial-system/backend/src/app/modules/defendant/civilClaimant.service.ts:75-95
Timestamp: 2024-10-17T11:45:23.482Z
Learning: The `created` field exists in the `CivilClaimant` model in `civilClaimant.model.ts`.
apps/judicial-system/backend/src/app/modules/defendant/test/civilClaimantController/createGuards.spec.ts (2)
Learnt from: gudjong
PR: island-is/island.is#16389
File: apps/judicial-system/backend/src/app/modules/file/guards/test/limitedAccessViewCaseFileGuard.spec.ts:175-185
Timestamp: 2024-10-17T11:53:19.983Z
Learning: In the Jest tests for the `LimitedAccessViewCaseFileGuard` in `apps/judicial-system/backend/src/app/modules/file/guards/test/limitedAccessViewCaseFileGuard.spec.ts`, code duplication in the `beforeEach` blocks is acceptable and should remain unchanged.
Learnt from: gudjong
PR: island-is/island.is#16389
File: apps/judicial-system/backend/src/app/modules/defendant/civilClaimant.service.ts:75-95
Timestamp: 2024-10-17T11:45:23.482Z
Learning: The `created` field exists in the `CivilClaimant` model in `civilClaimant.model.ts`.
apps/judicial-system/backend/src/app/modules/defendant/test/civilClaimantController/createRolesRules.spec.ts (1)
Learnt from: gudjong
PR: island-is/island.is#16389
File: apps/judicial-system/backend/src/app/modules/case/test/limitedAccessCaseController/getCaseFilesRecordPdfRolesRules.spec.ts:1-1
Timestamp: 2024-10-17T11:30:12.482Z
Learning: The usage of `defenderRule` in the following test files is intentional and should remain unchanged:
- `apps/judicial-system/backend/src/app/modules/file/test/limitedAccessFileController/createPresignedPostRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/file/test/limitedAccessFileController/deleteCaseFileRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/file/test/limitedAccessFileController/createCaseFileRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/file/test/limitedAccessFileController/getCaseFileSignedUrlRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/case/test/limitedAccessCaseController/getRulingPdfRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/case/test/limitedAccessCaseController/getRequestPdfRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/case/test/limitedAccessCaseController/getCourtRecordPdfRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/case/test/limitedAccessCaseController/getByIdRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/case/test/limitedAccessCaseController/getAllFilesZipRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/case/test/caseController/getAllRolesRules.spec.ts`
apps/judicial-system/backend/src/app/modules/defendant/test/civilClaimantController/deleteGuards.spec.ts (2)
Learnt from: gudjong
PR: island-is/island.is#16389
File: apps/judicial-system/backend/src/app/modules/file/guards/test/limitedAccessViewCaseFileGuard.spec.ts:175-185
Timestamp: 2024-10-17T11:53:19.983Z
Learning: In the Jest tests for the `LimitedAccessViewCaseFileGuard` in `apps/judicial-system/backend/src/app/modules/file/guards/test/limitedAccessViewCaseFileGuard.spec.ts`, code duplication in the `beforeEach` blocks is acceptable and should remain unchanged.
Learnt from: gudjong
PR: island-is/island.is#16389
File: apps/judicial-system/backend/src/app/modules/case/test/limitedAccessCaseController/getIndictmentPdfGuards.spec.ts:24-25
Timestamp: 2024-10-17T11:41:45.565Z
Learning: In certain scenarios within the judicial-system backend, the `RolesGuard` may intentionally follow the `CaseExistsGuard` when specific roles rules require the guard order to be reversed, as seen in tests like `getIndictmentPdfGuards.spec.ts`.
apps/judicial-system/backend/src/app/modules/defendant/test/civilClaimantController/deleteRolesRules.spec.ts (1)
Learnt from: gudjong
PR: island-is/island.is#16389
File: apps/judicial-system/backend/src/app/modules/case/test/limitedAccessCaseController/getCaseFilesRecordPdfRolesRules.spec.ts:1-1
Timestamp: 2024-10-17T11:30:12.482Z
Learning: The usage of `defenderRule` in the following test files is intentional and should remain unchanged:
- `apps/judicial-system/backend/src/app/modules/file/test/limitedAccessFileController/createPresignedPostRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/file/test/limitedAccessFileController/deleteCaseFileRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/file/test/limitedAccessFileController/createCaseFileRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/file/test/limitedAccessFileController/getCaseFileSignedUrlRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/case/test/limitedAccessCaseController/getRulingPdfRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/case/test/limitedAccessCaseController/getRequestPdfRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/case/test/limitedAccessCaseController/getCourtRecordPdfRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/case/test/limitedAccessCaseController/getByIdRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/case/test/limitedAccessCaseController/getAllFilesZipRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/case/test/caseController/getAllRolesRules.spec.ts`
apps/judicial-system/backend/src/app/modules/defendant/test/civilClaimantController/updateGuards.spec.ts (2)
Learnt from: gudjong
PR: island-is/island.is#16389
File: apps/judicial-system/backend/src/app/modules/file/guards/test/limitedAccessViewCaseFileGuard.spec.ts:175-185
Timestamp: 2024-10-17T11:53:19.983Z
Learning: In the Jest tests for the `LimitedAccessViewCaseFileGuard` in `apps/judicial-system/backend/src/app/modules/file/guards/test/limitedAccessViewCaseFileGuard.spec.ts`, code duplication in the `beforeEach` blocks is acceptable and should remain unchanged.
Learnt from: gudjong
PR: island-is/island.is#16389
File: apps/judicial-system/backend/src/app/modules/case/test/limitedAccessCaseController/getIndictmentPdfGuards.spec.ts:24-25
Timestamp: 2024-10-17T11:41:45.565Z
Learning: In certain scenarios within the judicial-system backend, the `RolesGuard` may intentionally follow the `CaseExistsGuard` when specific roles rules require the guard order to be reversed, as seen in tests like `getIndictmentPdfGuards.spec.ts`.
apps/judicial-system/backend/src/app/modules/defendant/test/civilClaimantController/updateRolesRules.spec.ts (1)
Learnt from: gudjong
PR: island-is/island.is#16389
File: apps/judicial-system/backend/src/app/modules/case/test/limitedAccessCaseController/getCaseFilesRecordPdfRolesRules.spec.ts:1-1
Timestamp: 2024-10-17T11:30:12.482Z
Learning: The usage of `defenderRule` in the following test files is intentional and should remain unchanged:
- `apps/judicial-system/backend/src/app/modules/file/test/limitedAccessFileController/createPresignedPostRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/file/test/limitedAccessFileController/deleteCaseFileRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/file/test/limitedAccessFileController/createCaseFileRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/file/test/limitedAccessFileController/getCaseFileSignedUrlRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/case/test/limitedAccessCaseController/getRulingPdfRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/case/test/limitedAccessCaseController/getRequestPdfRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/case/test/limitedAccessCaseController/getCourtRecordPdfRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/case/test/limitedAccessCaseController/getByIdRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/case/test/limitedAccessCaseController/getAllFilesZipRolesRules.spec.ts`
- `apps/judicial-system/backend/src/app/modules/case/test/caseController/getAllRolesRules.spec.ts`
apps/judicial-system/backend/src/app/modules/defendant/test/createTestingDefendantModule.ts (1)
Learnt from: gudjong
PR: island-is/island.is#16389
File: apps/judicial-system/backend/src/app/modules/defendant/civilClaimant.service.ts:75-95
Timestamp: 2024-10-17T11:45:23.482Z
Learning: The `created` field exists in the `CivilClaimant` model in `civilClaimant.model.ts`.
apps/judicial-system/backend/src/app/modules/notification/caseNotification.service.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-10-22T15:25:50.536Z
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/internalNotification.controller.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-10-22T15:25:50.536Z
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.
🪛 Biome
apps/judicial-system/backend/src/app/modules/defendant/test/civilClaimantController/create.spec.ts
[error] 41-41: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 42-42: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
apps/judicial-system/backend/src/app/modules/defendant/test/civilClaimantController/update.spec.ts
[error] 52-52: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 53-53: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
apps/judicial-system/backend/src/app/modules/notification/civilClaimantNotification.service.ts
[error] 146-158: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
Safe fix: Remove redundant block.
(lint/complexity/noUselessLoneBlockStatements)
🔇 Additional comments (42)
apps/judicial-system/backend/src/app/modules/defendant/guards/civilClaimaint.decorator.ts (1)
1-7: ```shell
#!/bin/bash
Let's verify if this is used in a REST controller and check its implementation
Check if this is a REST controller
rg -B 5 "@controller|@ApiTags" apps/judicial-system/backend/src/app/modules/notification/internalNotification.controller.ts
Check the method implementation where decorator is used
ast-grep --pattern 'async
$$$
}' apps/judicial-system/backend/src/app/modules/notification/internalNotification.controller.ts
Check if there are any GraphQL resolvers in the codebase
rg "@resolver|@query|@mutation" --type ts
</details>
<details>
<summary>apps/judicial-system/backend/src/app/modules/notification/dto/civilClaimantNotification.dto.ts (1)</summary>
`1-5`: **LGTM! Import statements are well-organized.**
The imports are properly grouped with external packages first, followed by internal imports, and each group is separated by a newline.
</details>
<details>
<summary>apps/judicial-system/backend/src/app/modules/defendant/index.ts (1)</summary>
`7-7`: **Verify if service export is necessary**
Based on previous learnings, services should only be exported when needed by other modules. Please verify if `CivilClaimantService` needs to be exposed outside this module.
<details>
<summary>✅ Verification successful</summary>
The search results show that `CivilClaimantService` is indeed used outside the defendant module, specifically in the case module for limited access case functionality. It's being imported and used in test files and the `limitedAccessCase.service.ts`. Let me verify if this is an actual dependency by checking the implementation.
---
**Export of CivilClaimantService is necessary**
The service is actively used by the case module, specifically in `limitedAccessCase.service.ts` where it calls `findLatestClaimantBySpokespersonNationalId`. This cross-module dependency justifies the export.
</details>
<details>
<summary>🏁 Scripts executed</summary>
The following scripts were executed for the analysis:
Script:
```shell
#!/bin/bash
# Search for CivilClaimantService usage outside the defendant module
rg "CivilClaimantService" --type ts --glob '!apps/judicial-system/backend/src/app/modules/defendant/**'
Length of output: 1679
Script:
#!/bin/bash
# Check the implementation to confirm actual usage
rg "civilClaimantService" -A 3 apps/judicial-system/backend/src/app/modules/case/limitedAccessCase.service.ts
Length of output: 528
apps/judicial-system/backend/src/app/modules/defendant/test/civilClaimantController/deleteRolesRules.spec.ts (2)
1-6: LGTM! Clean and focused imports.
The imports are well-organized, importing only the necessary rules and controller.
7-26: Verify the completeness of role-based access control testing.
The test correctly verifies that prosecutors and prosecutor representatives have delete permissions. However, it would be valuable to also test that other roles do not have these permissions.
Consider adding negative test cases to explicitly verify that other roles cannot delete civil claimants. This ensures the security of the delete operation.
apps/judicial-system/backend/src/app/modules/defendant/test/civilClaimantController/createGuards.spec.ts (2)
1-4: LGTM! Clean and focused imports.
The imports are well-organized, importing only the necessary guards and types needed for the tests.
17-24: Verify guard execution order importance.
The test verifies the presence and order of guards, but it's unclear if the order is significant for the business logic.
Let's check if there are dependencies between these guards:
✅ Verification successful
Guard order is significant and correctly enforced
The order of guards is important because:
-
CaseExistsGuardfetches and setsrequest.casewhich is required byCaseWriteGuard -
CaseWriteGuarddepends onrequest.casebeing present to verify user access permissions
The test correctly verifies this critical dependency chain across all controllers:
-
CaseExistsGuardmust run first to populate the case -
CaseWriteGuardmust run second to check permissions on the populated case
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for guard order dependencies in other controllers and documentation
# Search for CaseExistsGuard and CaseWriteGuard usage patterns
echo "Checking guard usage patterns across controllers:"
rg -A 5 "@UseGuards.*CaseExistsGuard.*CaseWriteGuard|@UseGuards.*CaseWriteGuard.*CaseExistsGuard"
# Search for any documentation about guard order
echo "Checking for guard order documentation:"
rg -l "CaseExistsGuard.*CaseWriteGuard" --type md
Length of output: 10692
Script:
#!/bin/bash
# Let's check the implementation of these guards to understand their dependencies
# Check CaseExistsGuard implementation
echo "=== CaseExistsGuard implementation ==="
ast-grep --pattern 'class CaseExistsGuard {
$$$
canActivate($$$) {
$$$
}
$$$
}'
# Check CaseWriteGuard implementation
echo "=== CaseWriteGuard implementation ==="
ast-grep --pattern 'class CaseWriteGuard {
$$$
canActivate($$$) {
$$$
}
$$$
}'
# Check if CaseWriteGuard depends on CaseExistsGuard
echo "=== CaseWriteGuard dependencies ==="
rg -B 3 -A 10 "class CaseWriteGuard"
Length of output: 1927
Script:
#!/bin/bash
# Let's check CaseExistsGuard implementation and its dependencies
echo "=== CaseExistsGuard implementation ==="
rg -B 3 -A 10 "class CaseExistsGuard"
# Check if request.case is set by CaseExistsGuard
echo "=== Checking where request.case is set ==="
rg -B 3 -A 10 "request.case ="
Length of output: 5938
apps/judicial-system/backend/src/app/modules/defendant/test/civilClaimantController/deleteGuards.spec.ts (3)
1-4: LGTM! Clean and focused imports.
The imports are well-organized, importing only the necessary guards and types needed for testing.
10-15: LGTM! Standard guard metadata retrieval.
The beforeEach setup follows the established pattern for guard testing, consistent with the learnings about test structure in the judicial system backend.
6-8: Verify guard ordering for delete operation security.
The guard order (CaseExistsGuard followed by CaseWriteGuard) appears correct, but let's verify this is the intended sequence for delete operations.
✅ Verification successful
Based on the search results, I can now provide a definitive response about the guard ordering for delete operations:
Guard ordering for delete operation is correctly implemented and consistent with security patterns.
The search results confirm that:
- The guard order
[CaseExistsGuard, CaseWriteGuard]is consistently used across delete operations in the codebase - This exact ordering is verified in multiple test files, particularly in
deleteGuards.spec.tsfiles - The same pattern is followed in other controllers that handle case-related operations requiring similar security checks
The ordering ensures that:
- Case existence is verified first (CaseExistsGuard)
- Write permissions are checked second (CaseWriteGuard)
This is the correct logical sequence as there's no point checking permissions if the case doesn't exist.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if there are any other delete operations with different guard ordering
# that might indicate a different security pattern.
# Search for delete method guard definitions in controllers
ast-grep --pattern 'class $_ {
$$$
@$$$Guard($$$)
delete($$$) {
$$$
}
$$$
}'
# Search for any security documentation about guard ordering
rg -l "guard.*order" -g "*.md" -g "*.txt"
Length of output: 136
Script:
#!/bin/bash
# Let's try a different approach to find guard usage patterns in delete operations
# Search for files containing delete operations in controllers
rg -l "delete.*\(" --type ts
# Then look for guard decorators in those files
rg "@.*Guard" -A 5 -B 5 --type ts | grep -A 5 -B 5 "delete"
# Also search specifically for these guard classes to understand their usage
rg "CaseExistsGuard|CaseWriteGuard" --type ts
Length of output: 106040
apps/judicial-system/backend/src/app/modules/defendant/test/civilClaimantController/updateGuards.spec.ts (3)
1-5: LGTM! Clean and focused imports.
The imports are well-organized, importing only the necessary guards and types needed for the tests.
6-9: LGTM! Guard order follows established patterns.
The guard order (CaseExistsGuard before CaseWriteGuard) aligns with the codebase's established patterns for access control.
10-15: LGTM! Clean metadata retrieval setup.
The beforeEach block efficiently retrieves guard metadata. The approach of using Reflect.getMetadata is the correct way to test decorator-applied guards.
apps/judicial-system/backend/src/app/modules/defendant/test/civilClaimantController/createRolesRules.spec.ts (3)
1-8: LGTM! Imports are well-organized.
The imports follow TypeScript best practices and include all necessary role rules for testing.
14-20: LGTM! Expected rules are well-defined.
The array includes all necessary roles that should have permission to create civil claimants.
22-27: LGTM! Test setup follows NestJS patterns.
The beforeEach hook correctly retrieves metadata for role-based permissions using Reflect API.
apps/judicial-system/backend/src/app/modules/defendant/test/civilClaimantController/updateRolesRules.spec.ts (2)
22-27: LGTM! Test setup is clean and follows best practices.
The beforeEach setup correctly retrieves role metadata for each test case, ensuring a clean state.
14-20: Verify if all required roles are included for civil claimant spokesperson assignments.
The expected rules array includes prosecutor and court-related roles, but we should verify if these are all the roles needed for the civil claimant spokesperson feature mentioned in the PR objectives.
✅ Verification successful
The expected rules array includes all necessary roles for civil claimant spokesperson assignments
The expected rules array in the test file correctly includes all the required roles for civil claimant spokesperson assignments:
- Prosecutor and prosecutor representative roles for managing case assignments
- District court judge, registrar, and assistant roles for court proceedings
The roles align with the civil claimant spokesperson feature's requirements, as these are the key roles involved in the judicial process. No additional roles are needed based on the codebase analysis, which shows these are the standard roles used across the system for similar operations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for role definitions related to civil claimants or spokespersons
rg -A 5 "civilClaimant|spokesperson" "apps/judicial-system/backend/src/app/guards"
# Search for other role rule test files to compare role patterns
fd -e ts -e spec.ts rolesRules.spec.ts apps/judicial-system/backend/src
Length of output: 156
Script:
#!/bin/bash
# Search for role-related constants and definitions in the codebase
rg -g '*.ts' -i "const.*Rule.*=.*" apps/judicial-system/backend/src/app/modules/defendant
# Look for role-related test files and their content
rg -g '*.spec.ts' "expectedRules" apps/judicial-system/backend/src/app/modules/defendant
# Search for civil claimant related files and content
rg -g '*.ts' "civilClaimant" apps/judicial-system/backend
Length of output: 34535
Script:
#!/bin/bash
# Search for role-related constants and their definitions
rg -g '*.ts' "const.*Rule.*=.*" apps/judicial-system/backend/src
# Search for role-related imports in the test file
rg -g '*.ts' "import.*Rule" apps/judicial-system/backend/src/app/modules/defendant/test/civilClaimantController/updateRolesRules.spec.ts
Length of output: 6072
apps/judicial-system/backend/src/app/modules/notification/civilClaimantNotification.strings.ts (1)
13-13:
Ensure HTML content is properly sanitized.
The message contains HTML tags (<br /> and potentially dynamic content in linkStart/linkEnd). Ensure that the notification rendering system properly sanitizes this HTML content to prevent XSS vulnerabilities.
✅ Verification successful
Let me gather more information about how these notification messages are used and rendered.
Let me search for the email service implementation to understand how the HTML content is handled.
Let me try searching for the email service in a different way and also look for any HTML-related utilities.
Based on the findings, I can now provide a final response:
HTML content in email notifications is properly handled
The notification system is designed to safely handle HTML content:
- The HTML content is part of a predefined message template using @formatjs/intl, not user-generated content
- The
<br />tags andlinkStart/linkEndplaceholders are controlled parts of the template - The messages are sent via a dedicated email service that processes these as email-safe HTML
- File names and other user inputs are sanitized using the
sanitizeutility from@island.is/judicial-system/formatters
The original review concern can be closed as the HTML content is properly managed within the system's email notification infrastructure.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for HTML sanitization in the notification rendering system
rg -g '!*.test.*' -g '!*.spec.*' -A 5 'sanitize|DOMPurify|escapeHtml|dangerouslySetInnerHTML' apps/judicial-system/
Length of output: 4358
Script:
#!/bin/bash
# Search for usage of these notification messages and how they are rendered
rg -g '!*.test.*' -g '!*.spec.*' -A 5 'civilClaimantNotification.strings' apps/judicial-system/
# Also search for email/notification rendering components
rg -g '!*.test.*' -g '!*.spec.*' -A 5 'sendEmail|renderEmail|NotificationTemplate|EmailTemplate' apps/judicial-system/
Length of output: 47980
Script:
#!/bin/bash
# Search for email service implementation and HTML sanitization
rg -g '!*.test.*' -g '!*.spec.*' -A 10 'class EmailService' apps/judicial-system/
# Also search for any HTML sanitization utilities
rg -g '!*.test.*' -g '!*.spec.*' -A 5 'sanitizeHtml|purifyHtml|cleanHtml' apps/judicial-system/
Length of output: 181
Script:
#!/bin/bash
# Search for email-related files and implementations
fd -e ts email apps/judicial-system/ | xargs rg -l 'sendEmail|createEmail|EmailService'
# Search for any HTML sanitization or security-related utilities
fd -e ts -e js sanitize apps/judicial-system/ | xargs cat
# Look for any security-related imports
rg -g '!*.test.*' -g '!*.spec.*' "from '@island.is/judicial-system/formatters'" -A 3 apps/judicial-system/
Length of output: 62011
apps/judicial-system/backend/src/app/modules/defendant/guards/civilClaimantExists.guard.ts (1)
1-12: LGTM! Clean and focused implementation.
The imports are minimal and the class structure follows NestJS best practices.
apps/judicial-system/backend/src/app/modules/notification/notification.module.ts (1)
Line range hint 30-43: LGTM: Module imports are well-organized
The module imports are properly structured with clear separation between external packages, internal modules, and local imports. The use of forwardRef for circular dependencies is appropriate.
apps/judicial-system/backend/src/app/modules/defendant/test/civilClaimantController/create.spec.ts (1)
1-80: Overall implementation looks good!
The test file is well-structured, follows Jest best practices, and provides good coverage of the basic scenarios. The suggested improvements would enhance the robustness of the tests, but the current implementation is acceptable.
🧰 Tools
🪛 Biome
[error] 41-41: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 42-42: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
libs/judicial-system/types/src/index.ts (1)
21-21: LGTM! The new export follows the established pattern.
The addition of CivilClaimantNotificationType is well-organized, grouped with other notification types, and follows the shared library's patterns for type exports.
Let's verify the implementation of the new notification type:
✅ Verification successful
The new notification type is properly implemented and used correctly
The verification confirms:
-
CivilClaimantNotificationTypeis properly defined as an enum withSPOKESPERSON_ASSIGNEDvalue - It's correctly used in:
- The notification service for sending spokesperson assigned notifications
- The civil claimant service for handling updates
- Corresponding test files with proper test coverage
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the CivilClaimantNotificationType implementation and its usage
# Test 1: Check if the notification type is properly defined
echo "Checking notification type definition..."
ast-grep --pattern 'enum CivilClaimantNotificationType {
$$$
}'
# Test 2: Check for proper usage in the notification service
echo "Checking usage in notification service..."
rg -A 5 "CivilClaimantNotificationType.SPOKESPERSON_ASSIGNED"
Length of output: 5905
libs/judicial-system/types/src/lib/notification.ts (1)
33-35: LGTM! Well-structured enum addition.
The new CivilClaimantNotificationType enum follows the established pattern and naming conventions, making it consistent with other notification types.
apps/judicial-system/backend/src/app/modules/defendant/civilClaimant.service.ts (2)
10-14: LGTM: Required imports for notification system.
The new imports properly support the notification system implementation for civil claimant spokespersons.
25-25: LGTM: Clean dependency injection.
The MessageService is properly injected following the established patterns.
apps/judicial-system/backend/src/app/modules/defendant/test/createTestingDefendantModule.ts (4)
16-17: LGTM: Import statements are well-organized.
The new imports for civil claimant components follow the established pattern and maintain consistency with the existing code structure.
Also applies to: 21-21
32-36: LGTM: Controller configuration is properly structured.
The CivilClaimantController is correctly added to the controllers array, maintaining consistency with the existing pattern.
74-74: LGTM: Service provider is correctly configured.
The CivilClaimantService is properly added to the providers array, maintaining the established dependency order.
62-72: Verify mock implementation completeness.
The CivilClaimant model token is correctly configured with standard Sequelize methods. Let's verify that all required methods are mocked.
✅ Verification successful
Mock implementation is complete and accurate
The verification confirms that all Sequelize methods used in the service are properly mocked:
- The service file uses
create,update,destroy, andfindOnemethods - All these methods are included in the mock implementation
- The unused methods
findAllandfindByPkare also mocked, following best practices for comprehensive mocking
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all Sequelize methods used in CivilClaimant service are mocked
# Search for Sequelize method calls in the CivilClaimant service
rg -t typescript "civilClaimant\." "apps/judicial-system/backend/src/app/modules/defendant/civilClaimant.service.ts"
Length of output: 151
Script:
#!/bin/bash
# Let's try different approaches to find the service file and its usage patterns
# First, find the correct path for the service file
fd -e ts civilClaimant
# Then search for Sequelize method calls in any relevant files
rg "civilClaimant.*\.(findOne|findAll|create|update|destroy|findByPk)" -t ts
# Also search for the model definition to understand required methods
ast-grep --pattern 'class CivilClaimant extends Model {
$$$
}'
Length of output: 16631
libs/judicial-system/message/src/lib/message.ts (2)
33-33: LGTM! Well-structured enum addition.
The new CIVIL_CLAIMANT_NOTIFICATION enum value follows the established naming convention and is appropriately placed at the end of the enum, maintaining backward compatibility.
70-70: LGTM! Consistent endpoint mapping.
The endpoint mapping 'civilClaimantNotification' follows the established camelCase pattern and maintains type safety through the index signature.
apps/judicial-system/backend/src/app/modules/defendant/test/civilClaimantController/update.spec.ts (2)
59-89: LGTM! Comprehensive test coverage for basic updates.
The test cases effectively verify both the update operation and the absence of notification messages for non-spokesperson updates.
91-123: LGTM! Well-structured tests for spokesperson notification.
The test cases effectively verify the spokesperson confirmation flow and notification queuing, aligning perfectly with the PR objectives.
✅ Verification successful
The search results show consistent usage of CivilClaimantNotificationType.SPOKESPERSON_ASSIGNED across the codebase, including:
- Type definition in
libs/judicial-system/types - Implementation in notification service
- Test coverage in both notification and civil claimant modules
- Proper usage in the civil claimant service
Let me verify the complete notification flow by checking the notification service implementation:
Let me verify the notification service implementation:
Let me check the complete notification flow by looking at the service implementation:
Verification complete - Notification implementation is correct
The test cases correctly verify the spokesperson notification flow. The service implementation shows:
- Proper type usage with
CivilClaimantNotificationType.SPOKESPERSON_ASSIGNED - Email notification logic with spokesperson validation
- Duplicate notification prevention through
shouldSendSpokespersonAssignedNotification - Error handling and logging
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify consistent notification type usage across the codebase
rg -A 2 "CivilClaimantNotificationType.SPOKESPERSON_ASSIGNED"
Length of output: 2955
Script:
#!/bin/bash
# Check the notification service implementation
ast-grep --pattern 'sendSpokespersonAssignedNotification($$$)'
Length of output: 121006
Script:
#!/bin/bash
# Check the civil claimant notification service implementation
rg -A 5 "sendSpokespersonAssignedNotification" apps/judicial-system/backend/src/app/modules/notification/civilClaimantNotification.service.ts
Length of output: 557
Script:
#!/bin/bash
# Get the full notification service implementation
cat apps/judicial-system/backend/src/app/modules/notification/civilClaimantNotification.service.ts
Length of output: 5191
apps/judicial-system/backend/src/app/modules/notification/test/createTestingNotificationModule.ts (1)
28-28: LGTM!
The import statement follows TypeScript conventions and maintains proper ordering.
apps/judicial-system/backend/src/app/modules/notification/internalNotification.controller.ts (2)
21-28: LGTM: Import statements are well-organized.
The new imports for civil claimant functionality are logically grouped and follow the established pattern.
Also applies to: 31-31, 38-38
54-54: LGTM: Service injection follows established pattern.
The CivilClaimantNotificationService is properly injected following the same pattern as other notification services.
apps/judicial-system/backend/src/app/modules/notification/civilClaimantNotification.service.ts (5)
30-48: Constructor implementation looks good
The constructor correctly initializes the service by injecting dependencies and calling the superclass constructor with appropriate arguments.
50-92: sendEmails method is well-implemented
The sendEmails method correctly constructs and sends emails to civil claimants, handling localization and formatting appropriately.
94-116: Notification sending conditions are correctly implemented
The shouldSendSpokespersonAssignedNotification method accurately determines whether a notification should be sent based on the spokesperson's email confirmation status and previous notifications.
118-139: sendSpokespersonAssignedNotification logic is sound
The method appropriately checks if a notification should be sent and calls sendEmails accordingly.
161-175: sendCivilClaimantNotification handles notification dispatching effectively
The method properly refreshes message formatting and manages error handling during notification sending.
...stem/backend/src/app/modules/defendant/test/civilClaimantController/updateRolesRules.spec.ts
Show resolved
Hide resolved
apps/judicial-system/backend/src/app/modules/notification/civilClaimantNotification.strings.ts
Show resolved
Hide resolved
apps/judicial-system/backend/src/app/modules/defendant/guards/civilClaimantExists.guard.ts
Outdated
Show resolved
Hide resolved
apps/judicial-system/backend/src/app/modules/notification/notification.module.ts
Outdated
Show resolved
Hide resolved
apps/judicial-system/backend/src/app/modules/defendant/civilClaimant.service.ts
Outdated
Show resolved
Hide resolved
...judicial-system/backend/src/app/modules/notification/test/createTestingNotificationModule.ts
Show resolved
Hide resolved
...tificationController/civilClaimantNotification/sendSpokespersonAssignedNotifications.spec.ts
Show resolved
Hide resolved
apps/judicial-system/backend/src/app/modules/notification/internalNotification.controller.ts
Outdated
Show resolved
Hide resolved
apps/judicial-system/backend/src/app/modules/notification/civilClaimantNotification.service.ts
Outdated
Show resolved
Hide resolved
…ithub.com/island-is/island.is into j-s/civil-claimant-assigned-notifications
apps/judicial-system/backend/src/app/modules/defendant/guards/civilClaimantExists.guard.ts
Show resolved
Hide resolved
apps/judicial-system/backend/src/app/modules/notification/caseNotification.service.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
apps/judicial-system/backend/src/app/modules/defendant/guards/test/civilClaimantExistsGuard.spec.ts (2)
18-38: Consider adding a return type to the canActivate mock.The test setup is well-structured, but the type safety could be improved.
Consider adding an explicit return type to the canActivate mock:
- then.result = guard.canActivate({ + then.result = guard.canActivate({ switchToHttp: () => ({ getRequest: mockRequest }), - } as unknown as ExecutionContext) + } as unknown as ExecutionContext) as boolean
86-116: Consider adding more edge cases.The current edge cases are well-tested, but consider adding tests for:
- Invalid UUID formats for caseId/civilClaimantId
- Case with multiple civil claimants
Would you like me to provide example test cases for these scenarios?
apps/judicial-system/backend/src/app/modules/defendant/civilClaimant.service.ts (1)
Line range hint
35-76: Well-structured notification system implementation.The architectural approach of using a message queue for notifications is solid:
- Maintains separation of concerns
- Allows for asynchronous notification processing
- Enables future extensibility for different notification types
Consider adding a notification configuration service in the future to manage notification rules and templates centrally.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
apps/judicial-system/backend/src/app/modules/defendant/civilClaimant.service.ts(4 hunks)apps/judicial-system/backend/src/app/modules/defendant/guards/civilClaimantExists.guard.ts(1 hunks)apps/judicial-system/backend/src/app/modules/defendant/guards/test/civilClaimantExistsGuard.spec.ts(1 hunks)apps/judicial-system/backend/src/app/modules/notification/civilClaimantNotification.service.ts(1 hunks)apps/judicial-system/backend/src/app/modules/notification/internalNotification.controller.ts(3 hunks)apps/judicial-system/backend/src/app/modules/notification/notification.module.ts(2 hunks)apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/sendAdvocateAssignedNotifications.spec.ts(0 hunks)
💤 Files with no reviewable changes (1)
- apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/sendAdvocateAssignedNotifications.spec.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/judicial-system/backend/src/app/modules/defendant/guards/civilClaimantExists.guard.ts
- apps/judicial-system/backend/src/app/modules/notification/civilClaimantNotification.service.ts
- apps/judicial-system/backend/src/app/modules/notification/internalNotification.controller.ts
- apps/judicial-system/backend/src/app/modules/notification/notification.module.ts
🧰 Additional context used
📓 Path-based instructions (2)
apps/judicial-system/backend/src/app/modules/defendant/civilClaimant.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/defendant/guards/test/civilClaimantExistsGuard.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 (1)
apps/judicial-system/backend/src/app/modules/defendant/guards/test/civilClaimantExistsGuard.spec.ts (1)
Learnt from: gudjong
PR: island-is/island.is#16389
File: apps/judicial-system/backend/src/app/modules/file/guards/test/limitedAccessViewCaseFileGuard.spec.ts:175-185
Timestamp: 2024-11-12T15:15:20.157Z
Learning: In the Jest tests for the `LimitedAccessViewCaseFileGuard` in `apps/judicial-system/backend/src/app/modules/file/guards/test/limitedAccessViewCaseFileGuard.spec.ts`, code duplication in the `beforeEach` blocks is acceptable and should remain unchanged.
🔇 Additional comments (5)
apps/judicial-system/backend/src/app/modules/defendant/guards/test/civilClaimantExistsGuard.spec.ts (3)
1-17: LGTM! Well-structured test setup with BDD pattern.
The imports are appropriate, and the GivenWhenThen interface provides a clear structure for behavior-driven test cases.
39-84: LGTM! Comprehensive test coverage for main scenarios.
The test cases effectively cover both successful and failure scenarios with appropriate assertions. The code duplication in beforeEach blocks follows the established pattern in similar guard tests, as noted in the learnings.
1-117: Verify integration with notification system.
The guard's tests align with the PR objective of implementing spokesperson notifications. However, we should verify the integration between this guard and the notification system.
✅ Verification successful
Let me gather more information about the interaction between the guard and notifications.
Guard is correctly integrated with the notification system
The CivilClaimantExistsGuard is properly integrated with the notification system:
- It's used in the notification controller's endpoint
/case/:caseId/civil-claimant-notification/:civilClaimantId - The guard validates the civil claimant's existence before the notification service processes the request
- The notification service handles spokesperson assignment notifications with proper error handling and delivery tracking
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for notification-related files and their interaction with this guard
rg -l "CivilClaimantExistsGuard" | xargs -I {} rg -l "Notification" {}
# Search for notification service usage in civil claimant-related files
ast-grep --pattern 'class $_ extends NotificationService {
$$$
}'
Length of output: 235
Script:
#!/bin/bash
# Check the notification controller implementation
cat "apps/judicial-system/backend/src/app/modules/notification/internalNotification.controller.ts"
# Look for civil claimant related routes using this guard
ast-grep --pattern 'controller($$$) {
$$$
@UseGuards($$$CivilClaimantExistsGuard$$$)
$$$
}'
# Search for any notification-related code in civil claimant context
rg -A 5 "notification" -g "*civil*"
Length of output: 16870
apps/judicial-system/backend/src/app/modules/defendant/civilClaimant.service.ts (2)
10-14: LGTM: Clean dependency injection and imports.
The new message-related imports and constructor dependency are properly organized and follow dependency injection best practices.
Also applies to: 25-25
75-75: Verify error handling in the update method.
While the await keyword is correctly used, we should ensure proper error handling for the notification sending.
✅ Verification successful
Error handling is sufficient for this update flow
The implementation shows appropriate error handling:
- Database update errors are handled with proper checks on
numberOfAffectedRows - The
sendUpdateCivilClaimantMessagesis a private method that only sends messages whenisSpokespersonConfirmedis true - Any errors from the message service will naturally propagate up to the caller
- The service already has error logging capabilities as seen in other methods
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for try-catch blocks in update methods across the codebase
ast-grep --pattern 'async update($$$) {
try {
$$$
} catch {
$$$
}
}'
Length of output: 84
Script:
#!/bin/bash
# Let's first locate the update method implementation
rg -A 10 "async update.*civilClaimant"
Length of output: 40
Script:
#!/bin/bash
# Let's try a broader search for the update method in the civilClaimant service
rg -A 15 "async update.*\(" apps/judicial-system/backend/src/app/modules/defendant/civilClaimant.service.ts
Length of output: 525
Script:
#!/bin/bash
# Let's get the complete update method implementation
rg -A 30 "async update.*\(" apps/judicial-system/backend/src/app/modules/defendant/civilClaimant.service.ts
Length of output: 1099
Script:
#!/bin/bash
# Let's check the implementation of sendUpdateCivilClaimantMessages
rg -A 15 "sendUpdateCivilClaimantMessages" apps/judicial-system/backend/src/app/modules/defendant/civilClaimant.service.ts
Length of output: 1111
apps/judicial-system/backend/src/app/modules/defendant/civilClaimant.service.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
apps/judicial-system/backend/src/app/modules/defendant/guards/test/civilClaimantExistsGuard.spec.ts (3)
11-14: Consider enhancing type safety with a more specific error type.The
Theninterface could be more specific about the error type rather than using the genericError.interface Then { result: boolean - error: Error + error: BadRequestException | NotFoundException | Error }
28-31: Improve type safety in guard activation test.The type casting to
ExecutionContextcould be made more explicit and type-safe.- } as unknown as ExecutionContext) + } as ExecutionContext & { switchToHttp: () => { getRequest: jest.Mock } })
39-116: LGTM! Consider adding edge cases for comprehensive coverage.The test scenarios provide good coverage of the main paths. Consider adding these edge cases:
- Case where civilClaimant ID exists but belongs to a different case
- Multiple civil claimants in the case array
Example structure for additional test:
describe('civil claimant from different case', () => { const caseId = uuid() const differentCaseId = uuid() const civilClaimantId = uuid() const civilClaimant = { id: civilClaimantId, caseId: differentCaseId } const theCase = { id: caseId, civilClaimants: [civilClaimant] } // ... test implementation })apps/judicial-system/backend/src/app/modules/defendant/civilClaimant.service.ts (1)
35-51: Add input validation and improve error handlingWhile the basic functionality is implemented correctly, consider these additional improvements:
- Add input validation for the updatedCivilClaimant parameter
- Consider using void instead of return for the message queue operation
- Add logging for successful message dispatch
private async sendUpdateCivilClaimantMessages( update: UpdateCivilClaimantDto, updatedCivilClaimant: CivilClaimant, ): Promise<void> { + if (!updatedCivilClaimant?.id || !updatedCivilClaimant?.caseId) { + throw new Error('Invalid civil claimant data'); + } if (update.isSpokespersonConfirmed === true) { - return this.messageService.sendMessagesToQueue([ + await this.messageService.sendMessagesToQueue([ { type: MessageType.CIVIL_CLAIMANT_NOTIFICATION, caseId: updatedCivilClaimant.caseId, body: { type: CivilClaimantNotificationType.SPOKESPERSON_ASSIGNED, }, elementId: updatedCivilClaimant.id, }, ]) + this.logger.debug( + `Sent spokesperson assignment notification for civil claimant ${updatedCivilClaimant.id}` + ); } }apps/judicial-system/backend/src/app/modules/notification/internalNotification.controller.ts (1)
136-162: LGTM with a documentation enhancement suggestionThe implementation follows the established pattern and includes proper validation guards. Consider enhancing the API description to be more specific about when and why these notifications are sent.
@ApiCreatedResponse({ type: DeliverResponse, - description: 'Sends civil claimant related notifications', + description: 'Sends notifications to confirmed spokespersons when they are officially assigned to indictment cases', })
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
apps/judicial-system/backend/src/app/modules/defendant/civilClaimant.service.ts(4 hunks)apps/judicial-system/backend/src/app/modules/defendant/guards/civilClaimantExists.guard.ts(1 hunks)apps/judicial-system/backend/src/app/modules/defendant/guards/test/civilClaimantExistsGuard.spec.ts(1 hunks)apps/judicial-system/backend/src/app/modules/notification/civilClaimantNotification.service.ts(1 hunks)apps/judicial-system/backend/src/app/modules/notification/internalNotification.controller.ts(3 hunks)apps/judicial-system/backend/src/app/modules/notification/notification.module.ts(2 hunks)apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/sendAdvocateAssignedNotifications.spec.ts(0 hunks)
💤 Files with no reviewable changes (1)
- apps/judicial-system/backend/src/app/modules/notification/test/internalNotificationController/sendAdvocateAssignedNotifications.spec.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/judicial-system/backend/src/app/modules/defendant/guards/civilClaimantExists.guard.ts
- apps/judicial-system/backend/src/app/modules/notification/civilClaimantNotification.service.ts
- apps/judicial-system/backend/src/app/modules/notification/notification.module.ts
🧰 Additional context used
📓 Path-based instructions (3)
apps/judicial-system/backend/src/app/modules/defendant/civilClaimant.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/defendant/guards/test/civilClaimantExistsGuard.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/internalNotification.controller.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 (2)
apps/judicial-system/backend/src/app/modules/defendant/guards/test/civilClaimantExistsGuard.spec.ts (1)
Learnt from: gudjong
PR: island-is/island.is#16389
File: apps/judicial-system/backend/src/app/modules/file/guards/test/limitedAccessViewCaseFileGuard.spec.ts:175-185
Timestamp: 2024-11-12T15:15:20.157Z
Learning: In the Jest tests for the `LimitedAccessViewCaseFileGuard` in `apps/judicial-system/backend/src/app/modules/file/guards/test/limitedAccessViewCaseFileGuard.spec.ts`, code duplication in the `beforeEach` blocks is acceptable and should remain unchanged.
apps/judicial-system/backend/src/app/modules/notification/internalNotification.controller.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 (6)
apps/judicial-system/backend/src/app/modules/defendant/civilClaimant.service.ts (3)
10-14: LGTM: Clean import organization
The new imports are well-organized and properly scoped from internal packages, supporting the notification system implementation.
25-25: LGTM: Proper dependency injection
The MessageService is correctly injected following the dependency injection pattern with appropriate access modifiers.
75-75: Verify test coverage for notification sending
The update method now includes notification functionality. Let's verify that the test coverage is adequate.
✅ Verification successful
Test coverage for notification sending is adequate
The update method's notification functionality is properly tested:
- Tests mock the MessageService for verification
- Test cases cover the spokesperson confirmation scenario
- Assertions verify that sendMessagesToQueue is called with correct parameters when spokesperson is confirmed
- Negative test cases verify that notifications are not sent in other scenarios
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for test files covering the update method and notification sending
rg -l "update.*civil.*claimant.*spokesperson" --type ts --glob "*test*"
# Look for specific test cases covering the notification scenario
ast-grep --pattern 'describe($_, $_) {
$$$
it($_, $_) {
$$$
update($_, $_)
$$$
sendMessagesToQueue
$$$
}
}'
Length of output: 209
Script:
#!/bin/bash
# Search for test files related to civil claimant service
fd "civilClaimant.*test.*\.ts$"
# Look for test cases in civil claimant service tests
rg -A 10 "describe.*CivilClaimant" --type ts
# Search for test cases covering sendUpdateCivilClaimantMessages
rg -A 10 "sendUpdateCivilClaimantMessages" --type ts
# Look for any test files containing update and civil claimant terms
rg -l "update.*civilClaimant" --type ts
Length of output: 16326
Script:
#!/bin/bash
# Look for test cases in update.spec.ts that cover the notification functionality
rg -A 20 "describe.*Update.*\(\)" apps/judicial-system/backend/src/app/modules/defendant/test/civilClaimantController/update.spec.ts
# Check for any mocking of message service in the test file
rg "mockMessageService" -A 5 apps/judicial-system/backend/src/app/modules/defendant/test/civilClaimantController/update.spec.ts
# Look for test cases specifically testing spokesperson confirmation
rg -A 10 "isSpokespersonConfirmed" apps/judicial-system/backend/src/app/modules/defendant/test/civilClaimantController/update.spec.ts
Length of output: 2200
apps/judicial-system/backend/src/app/modules/notification/internalNotification.controller.ts (3)
21-28: LGTM: Import statements are well-organized
The new imports for civil claimant functionality follow the existing pattern and include all necessary dependencies.
Also applies to: 31-31, 38-38
54-54: LGTM: Service injection follows established pattern
The civilClaimantNotificationService is properly injected following the same pattern as other notification services.
138-138: Verify MessageType enum constant
Ensure that the MessageType.CIVIL_CLAIMANT_NOTIFICATION constant is properly defined in the message types enum.
✅ Verification successful
CIVIL_CLAIMANT_NOTIFICATION constant is properly defined in the MessageType enum
The constant is correctly defined in libs/judicial-system/message/src/lib/message.ts at line 33 with the value 'CIVIL_CLAIMANT_NOTIFICATION'.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the MessageType enum includes CIVIL_CLAIMANT_NOTIFICATION
# Search for the MessageType enum definition
ast-grep --pattern 'enum MessageType {
$$$
CIVIL_CLAIMANT_NOTIFICATION = $_,
$$$
}'
Length of output: 3848
|
This PR currently has a merge conflict. Please resolve this and then re-add the |
…claimant-assigned-notifications
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
apps/judicial-system/backend/src/app/modules/notification/internalNotification.controller.ts (1)
142-145: Consider enhancing documentation and log consistency
The API documentation could be more detailed, similar to other endpoints. Consider adding information about:
- Expected request body format
- Possible response scenarios
- Use cases or examples
For consistency with other endpoints, consider updating the debug log message:
- `Sending ${notificationDto.type} notification for civil claimant ${civilClaimantId} and case ${caseId}`, + `Sending ${notificationDto.type} civil claimant notification for civil claimant ${civilClaimantId} and case ${caseId}`,Also applies to: 154-154
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
apps/judicial-system/backend/src/app/modules/notification/internalNotification.controller.ts(3 hunks)libs/judicial-system/types/src/lib/notification.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/judicial-system/types/src/lib/notification.ts
🧰 Additional context used
📓 Path-based instructions (1)
apps/judicial-system/backend/src/app/modules/notification/internalNotification.controller.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 (1)
apps/judicial-system/backend/src/app/modules/notification/internalNotification.controller.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 (3)
apps/judicial-system/backend/src/app/modules/notification/internalNotification.controller.ts (3)
21-28: LGTM: Import statements are well-organized
The new imports are properly organized and follow the existing pattern in the codebase. They provide all the necessary types and guards for the civil claimant notification functionality.
Also applies to: 31-31, 38-38
54-54: LGTM: Service injection follows established patterns
The civilClaimantNotificationService is properly injected following the same pattern as other notification services in the controller.
136-162: LGTM: Well-structured endpoint implementation
The new endpoint follows the established patterns in the controller:
- Proper use of guards for validation
- Consistent parameter handling
- Follows the same structure as other notification endpoints
* feat(j-s): Handle advocate assigned to defendant notification * Update defendant.service.ts * feat(j-s): Send civil claimant notification when assigned * Update defendantNotification.service.ts * fix(j-s): Small fix on modal texts * fix(j-s): Stop using advocate for defender emails * fix(j-s): remove advocate assigned from user roles rules * fix(j-s): remove and change tests * fix(j-s): Tests * test(j-s): Defendant notification tests * Update update.spec.ts * Update update.spec.ts * Update sendDefenderAssignedNotifications.spec.ts * Update defendantNotification.service.ts * test(j-s): Add tests * fix(j-s): Tests * test(j-s): Added more civil claimant tests * test(j-s): Added more civil claimant tests * Update civilClaimantNotification.service.ts * Update internalNotification.controller.ts * Update notification.module.ts * Update sendAdvocateAssignedNotifications.spec.ts * Update civilClaimant.service.ts * Update civilClaimant.service.ts * test(j-s): Civil claimant exists guard tests --------- Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> Co-authored-by: Guðjón Guðjónsson <gudjon@kolibri.is>
Senda póst á lögmenn bótakröfuhafa/réttargæslumann þegar viðkomandi er staðfestur í RVG.
What
Send notification to confirmed spokesperson in an indictment case
Why
So they know they've been assigned
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests