Implements full raw alert and contact point crud#1306
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1306 +/- ##
==========================================
+ Coverage 32.24% 32.55% +0.30%
==========================================
Files 754 755 +1
Lines 18827 18900 +73
Branches 3538 3551 +13
==========================================
+ Hits 6070 6152 +82
+ Misses 12154 12145 -9
Partials 603 603
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
WalkthroughThis update introduces a RESTful CRUD API for both "raw alerts" and "contact points" in the notifications module, replacing the previous alert controller and related tests. New repositories and controllers are added for robust data management, with comprehensive input/output validation and error handling. Associated test suites and environment configurations are updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ContactPointController
participant ContactPointRepository
participant RawAlertController
participant RawAlertRepository
%% Contact Point CRUD
Client->>ContactPointController: POST /contact-points (create)
ContactPointController->>ContactPointRepository: create(input)
ContactPointRepository-->>ContactPointController: ContactPointOutput
ContactPointController-->>Client: Ok(ContactPointOutputResponse)
Client->>ContactPointController: PATCH /contact-points/:id (update)
ContactPointController->>ContactPointRepository: updateById(id, input)
ContactPointRepository-->>ContactPointController: ContactPointOutput | undefined
ContactPointController-->>Client: Ok/Err(ContactPointOutputResponse or NotFound)
Client->>ContactPointController: GET /contact-points/:id (read)
ContactPointController->>ContactPointRepository: findById(id)
ContactPointRepository-->>ContactPointController: ContactPointOutput | undefined
ContactPointController-->>Client: Ok/Err(ContactPointOutputResponse or NotFound)
Client->>ContactPointController: DELETE /contact-points/:id (delete)
ContactPointController->>ContactPointRepository: deleteById(id)
ContactPointRepository-->>ContactPointController: ContactPointOutput | undefined
ContactPointController-->>Client: Ok/Err(ContactPointOutputResponse or NotFound)
%% Raw Alert CRUD
Client->>RawAlertController: POST /alerts/raw (create)
RawAlertController->>RawAlertRepository: create(input)
RawAlertRepository-->>RawAlertController: AlertOutput
RawAlertController-->>Client: Ok(AlertOutputResponse)
Client->>RawAlertController: PATCH /alerts/raw/:id (update)
RawAlertController->>RawAlertRepository: updateById(id, input)
RawAlertRepository-->>RawAlertController: AlertOutput | undefined
RawAlertController-->>Client: Ok/Err(AlertOutputResponse or NotFound)
Client->>RawAlertController: GET /alerts/raw/:id (read)
RawAlertController->>RawAlertRepository: findOneById(id)
RawAlertRepository-->>RawAlertController: AlertOutput | undefined
RawAlertController-->>Client: Ok/Err(AlertOutputResponse or NotFound)
Client->>RawAlertController: DELETE /alerts/raw/:id (delete)
RawAlertController->>RawAlertRepository: deleteOneById(id)
RawAlertRepository-->>RawAlertController: AlertOutput | undefined
RawAlertController-->>Client: Ok/Err(AlertOutputResponse or NotFound)
Poem
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
CodeRabbit Configuration File (
|
ygrishajev
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: 0
🧹 Nitpick comments (6)
apps/notifications/test/functional/alert-crud.spec.ts (2)
54-61: Consider testing more fields in the update operationThe update operation only tests setting
enabledto true, which is already the default value from line 37. This doesn't effectively test changing a value. Consider updating different fields likesummaryordescriptionto make this test more meaningful.async function shouldUpdate(alertId: string, app: INestApplication) { const res = await request(app.getHttpServer()) .patch(`/v1/alerts/raw/${alertId}`) - .send({ data: { enabled: true } }); + .send({ data: { summary: 'Updated summary', description: 'Updated description' } }); expect(res.status).toBe(200); - expect(res.body.data.enabled).toBe(true); + expect(res.body.data.summary).toBe('Updated summary'); + expect(res.body.data.description).toBe('Updated description'); }
63-70: Enhance the read validationThe read operation only verifies the ID, but it could verify more fields to ensure the complete object is correctly returned.
async function shouldRead(alertId: string, app: INestApplication) { const res = await request(app.getHttpServer()).get( `/v1/alerts/raw/${alertId}`, ); expect(res.status).toBe(200); expect(res.body.data.id).toBe(alertId); + expect(res.body.data).toMatchObject({ + id: alertId, + enabled: true, + summary: expect.any(String), + description: expect.any(String), + conditions: expect.any(Object), + createdAt: expect.any(String), + updatedAt: expect.any(String), + }); }apps/notifications/test/functional/contact-point-crud.spec.ts (2)
62-72: Enhance read validation to verify more fieldsSimilar to the raw alerts test, this test should verify more fields when reading a contact point to ensure the complete object structure is correctly returned.
async function shouldRead( contactPointId: string, app: INestApplication, ): Promise<void> { const res = await request(app.getHttpServer()).get( `/v1/contact-points/${contactPointId}`, ); expect(res.status).toBe(200); expect(res.body.data.id).toBe(contactPointId); + expect(res.body.data).toMatchObject({ + id: contactPointId, + userId: expect.any(String), + type: 'email', + config: { + addresses: expect.arrayContaining([expect.any(String)]), + }, + createdAt: expect.any(String), + updatedAt: expect.any(String), + }); }
1-112: Consider creating shared test utilities for CRUD operationsThis test file has a very similar structure to
alert-crud.spec.ts. You might want to create shared test utilities to reduce duplication and make future CRUD tests easier to implement.You could create a helper function that takes endpoint paths and entity-specific creation/update data as parameters, reducing duplicate test code across CRUD test files.
apps/notifications/src/modules/notifications/repositories/contact-point/contact-point.repository.ts (2)
43-83: Fix parameter naming in the toInput methodThere's an inconsistency in parameter naming in the
toInputmethod. The parameter is namedalertbut should becontactPointto maintain consistency with the class context.-private toInput<T extends Partial<InternalContactPointInput>>(alert: T): T { - if (alert.config) { +private toInput<T extends Partial<InternalContactPointInput>>(contactPoint: T): T { + if (contactPoint.config) { return { - ...alert, - config: contactPointConfigSchema.parse(alert.config), + ...contactPoint, + config: contactPointConfigSchema.parse(contactPoint.config), }; } - return alert; + return contactPoint; }
52-63: Consider restricting which fields can be updatedThe
updateByIdmethod currently allows updating any field, including potentially sensitive fields likeuserId. Consider adding restrictions on which fields can be updated, or add validation to prevent changing immutable fields.For example, you could add a validation step that removes sensitive fields from the input:
async updateById( id: string, input: Partial<ContactPointInput>, ): Promise<ContactPointOutput | undefined> { + // Prevent updating certain fields + const { userId, ...updateableFields } = input; + const [contactPoint] = await this.db .update(schema.ContactPoint) - .set(this.toInput(input)) + .set(this.toInput(updateableFields)) .where(eq(schema.ContactPoint.id, id)) .returning(); return contactPoint && this.toOutput(contactPoint); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
.commitlintrc.json(1 hunks)apps/notifications/env/.env.functional.test(1 hunks)apps/notifications/src/interfaces/all/all.module.ts(2 hunks)apps/notifications/src/interfaces/rest/controllers/alert/alert.controller.spec.ts(0 hunks)apps/notifications/src/interfaces/rest/controllers/alert/alert.controller.ts(0 hunks)apps/notifications/src/interfaces/rest/controllers/contact-point/contact-point.controller.spec.ts(1 hunks)apps/notifications/src/interfaces/rest/controllers/contact-point/contact-point.controller.ts(1 hunks)apps/notifications/src/interfaces/rest/controllers/raw-alert/raw-alert.controller.spec.ts(1 hunks)apps/notifications/src/interfaces/rest/controllers/raw-alert/raw-alert.controller.ts(1 hunks)apps/notifications/src/interfaces/rest/interceptors/http-result/http-result.interceptor.ts(0 hunks)apps/notifications/src/interfaces/rest/interceptors/http-validate/http-validate.interceptor.ts(1 hunks)apps/notifications/src/interfaces/rest/rest.module.ts(2 hunks)apps/notifications/src/modules/alert/repositories/raw-alert/raw-alert.repository.ts(3 hunks)apps/notifications/src/modules/notifications/model-schemas/contact-point.schema.ts(2 hunks)apps/notifications/src/modules/notifications/notifications.module.ts(1 hunks)apps/notifications/src/modules/notifications/repositories/contact-point/contact-point.repository.ts(2 hunks)apps/notifications/test/functional/alert-crud.spec.ts(3 hunks)apps/notifications/test/functional/contact-point-crud.spec.ts(1 hunks)
💤 Files with no reviewable changes (3)
- apps/notifications/src/interfaces/rest/interceptors/http-result/http-result.interceptor.ts
- apps/notifications/src/interfaces/rest/controllers/alert/alert.controller.spec.ts
- apps/notifications/src/interfaces/rest/controllers/alert/alert.controller.ts
🧰 Additional context used
🧬 Code Graph Analysis (4)
apps/notifications/src/modules/notifications/model-schemas/contact-point.schema.ts (1)
apps/notifications/src/lib/db/timestamps.ts (1)
timestamps(3-6)
apps/notifications/src/interfaces/rest/controllers/contact-point/contact-point.controller.spec.ts (2)
apps/notifications/src/interfaces/rest/controllers/contact-point/contact-point.controller.ts (4)
contactPointCreateInputSchema(21-25)contactPointOutputSchema(27-31)contactPointPatchInputSchema(40-43)contactPointNotFoundError(45-47)apps/notifications/test/mocks/provider.mock.ts (1)
MockProvider(4-8)
apps/notifications/test/functional/alert-crud.spec.ts (3)
apps/notifications/src/interfaces/rest/controllers/raw-alert/raw-alert.controller.ts (1)
alertCreateInputSchema(21-29)apps/notifications/src/config/db.config.ts (1)
DRIZZLE_PROVIDER_TOKEN(1-1)apps/notifications/test/seeders/contact-point.seeder.ts (1)
generateContactPoint(7-23)
apps/notifications/src/interfaces/rest/controllers/raw-alert/raw-alert.controller.ts (2)
apps/notifications/src/interfaces/rest/interceptors/http-validate/http-validate.interceptor.ts (1)
ValidateHttp(31-42)apps/notifications/src/modules/alert/repositories/raw-alert/raw-alert.repository.ts (1)
AlertOutput(46-48)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: validate-notifications
🔇 Additional comments (37)
.commitlintrc.json (1)
28-29: Added "contact-point" to allowed commit scopes - looks goodThis addition correctly extends the commit lint scope rules to include "contact-point", which aligns with the new feature being implemented in this PR. The syntax maintains the proper comma separation between list items.
apps/notifications/env/.env.functional.test (1)
5-6: Environment variables for Novu integration look goodThese new environment variables for Novu integration (secret key and mailer workflow ID) are appropriately configured with test placeholder values for the functional test environment, supporting the new notification features.
apps/notifications/src/interfaces/all/all.module.ts (2)
8-8: Import of RawAlertController correctly replaces AlertControllerThe import statement properly brings in the new RawAlertController, consistent with the PR's objective to implement full raw alert CRUD.
34-34: Controller registration updated appropriatelyThe controllers array now correctly includes RawAlertController instead of the previous AlertController, completing the module-level changes needed for the new implementation.
apps/notifications/src/modules/notifications/notifications.module.ts (1)
48-52: Exports expanded to support new functionalityThe module's exports are appropriately expanded to include ContactPointRepository and EmailSenderService along with the previously exported NotificationRouterService. This change properly exposes the necessary services for the new raw alert and contact point CRUD operations.
apps/notifications/src/modules/notifications/model-schemas/contact-point.schema.ts (1)
2-4: Good refactoring to use shared timestamp columnsThe change improves code maintainability by replacing explicit timestamp column definitions with the centralized
timestampsutility. This follows the DRY principle and ensures consistent timestamp definitions across schemas.Also applies to: 16-17
apps/notifications/src/interfaces/rest/rest.module.ts (1)
6-9: Module configuration properly updated for new controllersThe module has been correctly updated to register the new
RawAlertControllerandContactPointController, and imports the requiredNotificationsModuleto provide dependencies for these controllers. This aligns with the PR objective of implementing full raw alert CRUD operations.Also applies to: 20-20, 22-22
apps/notifications/src/interfaces/rest/interceptors/http-validate/http-validate.interceptor.ts (1)
121-134: Improved Result type handling in response validationThe interceptor now correctly handles
Resulttypes from ts-results library by:
- Short-circuiting validation when there's no response schema or when data is already an
Errresult- Properly unwrapping
Okresults for validation- Maintaining appropriate error logging
This change ensures proper propagation of error results without attempting unnecessary validation.
apps/notifications/src/interfaces/rest/controllers/contact-point/contact-point.controller.spec.ts (1)
1-137: Well-structured and comprehensive test suiteThe test suite thoroughly covers all CRUD operations (create, patch, get, delete) with both success and error scenarios. Good use of:
- Zod schema mock generators for test data
- Proper assertion of both repository method calls and result types
- Clear test organization with descriptive test cases
- Clean test setup with a reusable helper function
apps/notifications/test/functional/alert-crud.spec.ts (1)
17-27: Clean test structure following best practices!Good use of modularized test helpers for each CRUD operation, making the test more readable and maintainable.
apps/notifications/test/functional/contact-point-crud.spec.ts (1)
12-22: Well-structured test patternGood job organizing the test with a clear CRUD lifecycle and using helper functions for each operation.
apps/notifications/src/interfaces/rest/controllers/raw-alert/raw-alert.controller.spec.ts (2)
19-141: Well-structured and comprehensive controller testsThe tests thoroughly cover all CRUD operations and both success and error cases. Good job using mocks to test controller behavior in isolation from the repository implementation.
36-65: Thorough testing of error handlingExcellent job testing both successful updates and the "not found" error case in the patchAlert method.
apps/notifications/src/modules/notifications/repositories/contact-point/contact-point.repository.ts (1)
43-72: Good implementation of full CRUD operationsThe implementation of the CRUD operations is clean and follows a consistent pattern. The validation of input data using the schema ensures data integrity.
apps/notifications/src/modules/alert/repositories/raw-alert/raw-alert.repository.ts (5)
4-4: Import updated to includeeq.Good addition of the
eqimport from drizzle-orm, which is now used in the new repository methods for equality filtering.
92-103: Well-implemented update method with proper access control.The
updateByIdmethod is well-structured:
- It accepts a partial input, allowing for targeted updates
- Uses
whereAccessibleByto enforce access control filters- Returns the updated entity or undefined if not found
105-111: Clean implementation of the findOneById method.This method properly queries a single record by ID and applies access control filtering via
whereAccessibleBy.
113-120: Properly implemented delete method with access control.The
deleteOneByIdmethod correctly:
- Applies access control filtering
- Returns the deleted entity (useful for confirmation messages and auditing)
- Handles the case where no entity is found
164-173: Smart conditional parsing for partial updates.The updated
toInputmethod:
- Now accepts partial inputs through generic typing
- Only parses the
conditionsfield when it's present- This elegantly handles partial updates while maintaining validation
This is especially important for the PATCH endpoint where only partial data is provided.
apps/notifications/src/interfaces/rest/controllers/raw-alert/raw-alert.controller.ts (9)
21-29: Well-structured input validation schema.The alert creation schema includes all necessary fields with appropriate validation:
- UUID validation for IDs
- Minimum length requirements for text fields
- Type-specific validation using Zod
22-23: Consider implementing authentication-based user ID.The TODO comment indicates that the user ID should come from authentication rather than being passed in the request body.
This is a security best practice to prevent unauthorized users from creating alerts for other users. Consider prioritizing this implementation.
31-39: Good response schema structure.The output schema properly extends the input schema and adds the expected metadata fields (id, timestamps). Wrapping the response in a
dataproperty follows API design best practices.
41-47: Appropriate patch schema with optional fields.All fields in the patch schema are correctly marked as optional, which is essential for PATCH operations. The validation requirements are maintained for each field when present.
58-69: Well-implemented create endpoint.The create method:
- Uses proper validation via the
ValidateHttpdecorator- Follows the REST pattern with POST verb
- Returns a wrapped response in the expected format
- Handles async operations correctly
71-80: Properly implemented read endpoint.The get method correctly:
- Uses path parameters for the ID
- Returns a type-safe Result with proper error handling
- Uses the shared response transformation logic
82-93: Well-structured update endpoint.The patch method:
- Uses correct HTTP verb (PATCH)
- Has proper input validation
- Propagates not-found errors appropriately
- Follows REST conventions
95-104: Correctly implemented delete endpoint.The delete method properly:
- Uses the correct HTTP verb
- Returns the deleted entity in the response
- Handles the not-found case appropriately
106-110: Efficient response helper method.The private
toResponsemethod removes duplication across endpoints and consistently handles the not-found case. Good use of the Result type to represent success/failure outcomes.apps/notifications/src/interfaces/rest/controllers/contact-point/contact-point.controller.ts (9)
21-25: Good input validation schema for contact points.The contact point creation schema appropriately:
- Validates UUID for user ID
- Restricts the type field to a literal value
- Uses a detailed config schema imported from the repository
27-38: Well-structured response schema.The output schema properly extends the input schema with metadata fields and follows the same pattern as the alert controller, showing consistency in API design.
40-43: Appropriate patch schema with optional fields.The patch schema correctly marks all fields as optional while maintaining their validation rules.
49-56: Consistent controller structure.The controller follows the same pattern as the raw-alert controller:
- Versioned API with clean path naming
- Dependency injection for repository access
- Consistent method naming and parameter handling
58-69: Well-implemented create endpoint.The create endpoint uses proper validation, follows REST conventions, and returns a correctly structured response.
71-80: Properly implemented read endpoint.The get endpoint correctly handles path parameters, validation, and error cases.
82-93: Well-structured update endpoint.The patch endpoint correctly implements partial updates with validation and error handling.
95-104: Correctly implemented delete endpoint.The delete endpoint follows REST conventions and properly handles the response and error cases.
106-112: Clean response transformation method.The private
toResponsemethod efficiently handles the common pattern of transforming repository results into HTTP responses or errors.
ygrishajev
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
apps/notifications/src/modules/notifications/repositories/contact-point/contact-point.repository.ts (3)
52-60: Alignidparameter type with the entity’s declared shape
updateById(anddeleteByIdbelow) receiveid: string, whilefindByIdalready leverages the stricterContactPointOutput['id']alias.
Using the same alias everywhere keeps refactors safe (e.g. if the PK ever becomesnumber) and makes the API self-documenting.- async updateById( - id: string, + async updateById( + id: ContactPointOutput['id'],Do the same for
deleteById.
56-60: Guard against empty PATCH payloads
db.update().set(this.toInput(input))will generateSETwith an empty object when the caller sends{ data: {} }, leading to invalid SQL on some drivers.
Consider short-circuiting whenObject.keys(input).length === 0, or validate at the controller level that at least one mutable field is present.
74-83: Minor naming nit – avoid “alert” in contact-point helper
toInputstill refers to its parameter asalert, which can mislead future readers.-private toInput<T extends Partial<InternalContactPointInput>>(alert: T): T { - if (alert.config) { - return { - ...alert, - config: contactPointConfigSchema.parse(alert.config), +private toInput<T extends Partial<InternalContactPointInput>>(cp: T): T { + if (cp.config) { + return { + ...cp, + config: contactPointConfigSchema.parse(cp.config), }; } - return alert; + return cp; }apps/notifications/src/interfaces/rest/controllers/contact-point/contact-point.controller.ts (2)
58-69: Return 201 Created for successful POSTThe REST convention is to signal resource creation with status 201. Adding
@HttpCode(201)(or@HttpStatus.CREATED) makes this explicit and helps API consumers.@Post() +@HttpCode(201) @ValidateHttp({
95-104: Consider 204 No Content for deletions
deleteContactPointreturns the deleted entity, but many APIs answer with 204 and an empty body to avoid leaking now-orphaned data back to the client.
If the consumer truly needs the object, keep it; otherwise, change the response contract and add@HttpCode(204).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
.commitlintrc.json(1 hunks)apps/notifications/env/.env.functional.test(1 hunks)apps/notifications/package.json(1 hunks)apps/notifications/src/interfaces/rest/controllers/contact-point/contact-point.controller.spec.ts(1 hunks)apps/notifications/src/interfaces/rest/controllers/contact-point/contact-point.controller.ts(1 hunks)apps/notifications/src/interfaces/rest/rest.module.ts(2 hunks)apps/notifications/src/modules/notifications/model-schemas/contact-point.schema.ts(2 hunks)apps/notifications/src/modules/notifications/notifications.module.ts(1 hunks)apps/notifications/src/modules/notifications/repositories/contact-point/contact-point.repository.ts(2 hunks)apps/notifications/test/functional/contact-point-crud.spec.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/notifications/package.json
🚧 Files skipped from review as they are similar to previous changes (7)
- apps/notifications/env/.env.functional.test
- .commitlintrc.json
- apps/notifications/src/modules/notifications/notifications.module.ts
- apps/notifications/src/modules/notifications/model-schemas/contact-point.schema.ts
- apps/notifications/src/interfaces/rest/rest.module.ts
- apps/notifications/test/functional/contact-point-crud.spec.ts
- apps/notifications/src/interfaces/rest/controllers/contact-point/contact-point.controller.spec.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
apps/notifications/src/interfaces/rest/controllers/contact-point/contact-point.controller.ts (2)
apps/notifications/src/modules/notifications/repositories/contact-point/contact-point.repository.ts (2)
contactPointConfigSchema(10-12)ContactPointOutput(22-24)apps/notifications/src/interfaces/rest/interceptors/http-validate/http-validate.interceptor.ts (1)
ValidateHttp(31-42)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: validate-notifications
apps/notifications/src/interfaces/rest/controllers/contact-point/contact-point.controller.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/notifications/src/interfaces/rest/controllers/contact-point/contact-point.controller.ts (1)
102-108: Proper instantiation of NotFoundException per requestGood implementation of the error handling in the
toResponsemethod. You're creating a newNotFoundExceptioninstance for each "not found" case, which addresses the issue mentioned in the previous review comment about avoiding singleton error instances.
🧹 Nitpick comments (5)
apps/notifications/src/interfaces/rest/controllers/contact-point/contact-point.controller.ts (5)
21-25: Consider allowing for future contact point typesThe contact point creation schema currently only allows for
'email'as a type. If you plan to support additional contact point types in the future (like SMS, Slack, etc.), consider using a union type instead of a literal.- type: z.literal('email'), + type: z.enum(['email']), // Can be expanded to z.enum(['email', 'sms', 'slack']) later
54-65: Narrow down the error type for better type safetyThe
createContactPointmethod returnsResult<ContactPointOutputResponse, unknown>, usingunknownas the error type. Consider using a more specific union of error types to improve type safety and API documentation.- async createContactPoint(...): Promise<Result<ContactPointOutputResponse, unknown>> { + async createContactPoint(...): Promise<Result<ContactPointOutputResponse, BadRequestException | InternalServerErrorException>> {
67-76: Consider validating ID parameter formatThe ID parameter is used directly without validation. Consider adding validation for the UUID format at the controller level to provide better error messages.
@Get(':id') @ValidateHttp({ + params: z.object({ id: z.string().uuid() }), response: contactPointOutputResponseSchema, })
91-100: Ensure appropriate status code for successful deletionThe delete operation returns the deleted entity, which is good practice. However, make sure the endpoint returns the appropriate status code (204 No Content or 200 OK with the deleted resource). Based on your interceptor implementation, confirm it's handling this correctly.
45-109: Add OpenAPI decorators for API documentationConsider adding OpenAPI decorators to document the API endpoints, request/response schemas, and possible error responses. This will make API exploration and integration easier.
import { ApiTags, ApiOperation, ApiResponse, ApiBody, ApiParam } from '@nestjs/swagger'; @ApiTags('contact-points') @Controller({...}) export class ContactPointController { @ApiOperation({ summary: 'Create a new contact point' }) @ApiBody({ schema: { ... } }) @ApiResponse({ status: 201, description: 'Created successfully' }) @Post() async createContactPoint(...) {...} // Other methods... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/notifications/src/interfaces/rest/controllers/contact-point/contact-point.controller.spec.ts(1 hunks)apps/notifications/src/interfaces/rest/controllers/contact-point/contact-point.controller.ts(1 hunks)apps/notifications/src/interfaces/rest/controllers/raw-alert/raw-alert.controller.spec.ts(1 hunks)apps/notifications/src/interfaces/rest/controllers/raw-alert/raw-alert.controller.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/notifications/src/interfaces/rest/controllers/contact-point/contact-point.controller.spec.ts
- apps/notifications/src/interfaces/rest/controllers/raw-alert/raw-alert.controller.spec.ts
- apps/notifications/src/interfaces/rest/controllers/raw-alert/raw-alert.controller.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: validate-notifications
🔇 Additional comments (3)
apps/notifications/src/interfaces/rest/controllers/contact-point/contact-point.controller.ts (3)
33-38: Response format follows consistent data wrapper patternThe use of a standardized response wrapper with a
dataproperty ensures consistent API responses throughout the application, which aligns well with REST API best practices.
78-89: Consider handling potential validation failures in updateByIdWhen updating a contact point, repository validation failures might occur beyond "not found" cases (e.g., invalid config format). Consider handling these potential errors or ensuring they're properly propagated to the client.
Can you verify how validation errors from the repository are currently handled in the system?
1-109: Well-structured controller with proper separation of concernsOverall, this controller is well-designed with clear separation of concerns:
- Input/output schemas are defined using Zod for validation
- HTTP methods are appropriately mapped to CRUD operations
- The repository pattern is correctly implemented with dependency injection
- Error handling follows a consistent pattern with Result types
- The code structure is clean and maintainable
This implementation provides a solid foundation for managing contact points in the system.
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores
Refactor