feat(fa): Creating endpoint for open-api that retrieves one application from id #16490
Conversation
This reverts commit d9691c5.
…g-pdf-endpoint-to-api
…g-pdf-endpoint-to-api
WalkthroughThe pull request introduces new methods across various controllers and services to enhance the application retrieval functionality. Specifically, a Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
apps/financial-aid/open-api/src/app/app.controller.ts (1)
1-1: Remove unused importParamThe
Paramdecorator is imported but not used in this file. To maintain clean and efficient code, it's recommended to remove unused imports.Apply this diff to remove the unused import:
-import { Controller, Get, Headers, Inject, Param, Query } from '@nestjs/common' +import { Controller, Get, Headers, Inject, Query } from '@nestjs/common'apps/financial-aid/backend/src/app/modules/openApiApplications/openApiApplication.controller.ts (1)
55-69: LGTM: New getById method is well-implemented.The new
getByIdmethod follows NestJS conventions and is consistent with the existing code style. Good use of Swagger documentation and logging.Consider changing the service method call for consistency:
- return this.applicationService.getbyID(municipalityCode, id) + return this.applicationService.getById(municipalityCode, id)This ensures consistent naming convention (camelCase) across the codebase.
apps/financial-aid/open-api/src/app/app.service.ts (1)
65-89: Enhance error handling and logging in the newgetApplicationmethod.The implementation of the new
getApplicationmethod is generally good and consistent with the existing code. However, there are a couple of areas for improvement:
- Error Handling: Consider adding try-catch block to handle potential network or API errors.
- Logging: The log message could be more specific to indicate it's fetching a single application.
Here's a suggested improvement:
async getApplication( apiKey: string, municipalityCode: string, id: string, ): Promise<ApplicationModel> { this.logger.info( `Attempting to fetch application with ID ${id} for municipality ${municipalityCode}`, ) const url = new URL( `${this.config.backend.url}/api/financial-aid/open-api-applications/id/${id}`, ) try { const response = await fetch(url, { method: 'GET', headers: { 'Content-Type': 'application/json', 'API-Key': apiKey, 'Municipality-Code': municipalityCode, }, }) if (!response.ok) { throw new Error(`HTTP error! status: ${response.status}`) } return await response.json() } catch (error) { this.logger.error(`Error fetching application with ID ${id}: ${error.message}`) throw error } }This implementation improves error handling and provides more specific logging.
Overall, the new method is well-implemented and follows TypeScript best practices. It's a good addition to the service.
apps/financial-aid/backend/src/app/modules/openApiApplications/openApiApplication.service.ts (3)
99-170: LGTM! Consider adding error handling.The
getbyIDmethod is well-implemented and consistent with the existinggetAllmethod. It follows TypeScript and NestJS best practices.Consider adding error handling for cases where no application is found. For example:
const application = await this.applicationModel.findOne({ // ... existing query ... }); if (!application) { throw new NotFoundException(`Application with ID ${id} not found`); } return application;This will provide a more informative response when an application is not found.
99-102: Consider using a more specific type formunicipalityCodes.If
municipalityCodesis always expected to be a single code, consider using a more specific type or renaming the parameter for clarity.async getbyID( municipalityCode: string, id: string, ): Promise<ApplicationModel>
107-109: Add a comment explaining the state filtering.It would be helpful to add a brief comment explaining why only REJECTED or APPROVED applications are being retrieved.
// Only retrieve applications that have been fully processed (REJECTED or APPROVED) state: { [Op.or]: [ApplicationState.REJECTED, ApplicationState.APPROVED], },
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- apps/financial-aid/backend/src/app/modules/openApiApplications/openApiApplication.controller.ts (2 hunks)
- apps/financial-aid/backend/src/app/modules/openApiApplications/openApiApplication.service.ts (1 hunks)
- apps/financial-aid/open-api/src/app/app.controller.ts (2 hunks)
- apps/financial-aid/open-api/src/app/app.service.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
apps/financial-aid/backend/src/app/modules/openApiApplications/openApiApplication.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/financial-aid/backend/src/app/modules/openApiApplications/openApiApplication.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/financial-aid/open-api/src/app/app.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/financial-aid/open-api/src/app/app.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."
🔇 Additional comments (3)
apps/financial-aid/open-api/src/app/app.controller.ts (1)
38-55:⚠️ Potential issueImprove method implementation and documentation
The
getPdfmethod has a few areas for improvement:
- The method name
getPdfdoesn't accurately reflect its current functionality, as it's not returning a PDF.- The
@ApiCreatedResponsedecorator specifies an incorrect return type. It should beApplicationModelinstead of[ApplicationBackendModel].- Error handling is not implemented, which could lead to unhandled exceptions.
Consider applying the following changes:
- Rename the method to better reflect its current functionality, e.g.,
getApplicationById.- Update the
@ApiCreatedResponsedecorator to accurately reflect the return type.- Implement error handling using try-catch or NestJS exception filters.
Here's a suggested implementation:
@Get('application') @ApiCreatedResponse({ type: ApplicationModel, description: 'Gets a single application by ID', }) async getApplicationById( @Headers('API-Key') apiKey: string, @Headers('Municipality-Code') municipalityCode: string, @Query('id') id: string, ): Promise<ApplicationModel> { this.logger.info(`Fetching application with ID: ${id}`) try { const application = await this.appService.getApplication(apiKey, municipalityCode, id); this.logger.info(`Application fetched successfully`) return application; } catch (error) { this.logger.error(`Error fetching application: ${error.message}`) throw new NotFoundException(`Application with ID ${id} not found`); } }To ensure that the
getApplicationmethod inAppServiceis correctly implemented, let's verify its implementation:apps/financial-aid/backend/src/app/modules/openApiApplications/openApiApplication.controller.ts (2)
1-8: LGTM: Import statements are correctly updated.The addition of
Paramto the import statement is consistent with its usage in the newgetByIdmethod and follows the existing code style.
Line range hint
1-69: Changes align well with PR objectives and maintain code quality.The new
getByIdendpoint successfully implements the functionality to retrieve a single application by ID, as outlined in the PR objectives. The implementation follows TypeScript and NestJS best practices, maintaining consistency with the existing codebase.To ensure the new endpoint is properly integrated, please run the following verification script:
This script will help verify the proper implementation of the
getByIdmethod in the service layer and check for appropriate error handling.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16490 +/- ##
==========================================
+ Coverage 36.76% 36.77% +0.01%
==========================================
Files 6847 6845 -2
Lines 141884 141768 -116
Branches 40429 40381 -48
==========================================
- Hits 52167 52139 -28
+ Misses 89717 89629 -88
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 14 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
There was a problem hiding this comment.
LGTM. However, I wonder if this new endpoint will ever be used. Wasn't the idea that the external system would call the getAll endpoint, loop through the responses form that endpoint and finally call the yet to be created endpoint to retrieve the PDF for a single application?
Datadog ReportAll test runs ✅ 4 Total Test Services: 0 Failed, 4 Passed Test Services
🔻 Code Coverage Decreases vs Default Branch (2) |
…on from id (#16490) * adding sortable feature * Revert "adding sortable feature" This reverts commit d9691c5. * adding more detail for api * removing white space break just adding html element to the db * adding children to api * removing extra imports * creating pdf endpoint and returning json of application
...
Attach a link to issue if relevant
What
Why
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation