fix(skilavottord): Handle null value in model year #16220
Conversation
WalkthroughThe pull request introduces several changes across multiple files, primarily focusing on enhancing input handling and error management. Key modifications include adding a null check in the 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: 1
🧹 Outside diff range and nitpick comments (2)
apps/skilavottord/web/utils/dateUtils.ts (1)
23-27: Approved with suggestions for improvementThe changes successfully address the PR objective of handling null values for model years. Good job on improving the robustness of the function. Here are some suggestions for further improvement:
Consider updating the type annotation for better type safety:
export const getYear = (dateTime: string | null | undefined): string => {Update the return type annotation to explicitly include the empty string:
export const getYear = (dateTime: string | null | undefined): string => {You could simplify the conditional check using the nullish coalescing operator:
export const getYear = (dateTime: string | null | undefined): string => { return dateTime ? format(new Date(dateTime), 'yyyy') : ''; }These changes will enhance type safety and make the function more concise while maintaining the desired behavior.
libs/clients/car-recycling/src/lib/carRecyclingClient.service.ts (1)
94-94: Approved: Improved error messages with a suggestion for consistencyThe error messages have been corrected for grammar, enhancing clarity. The use of
permno.slice(-3)in the error messages is a good practice for identifying the specific vehicle without exposing the full permit number.For consistency, consider using a template literal for the error message on line 94, similar to line 99:
throw new Error(`Failed to create vehicle ${permno.slice(-3)}`)This would make both error messages identical and easier to maintain.
Also applies to: 99-99
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
- apps/skilavottord/web/utils/dateUtils.ts (1 hunks)
- apps/skilavottord/ws/src/app/modules/vehicle/dto/createVehicle.input.ts (1 hunks)
- libs/application/template-api-modules/src/lib/modules/templates/car-recycling/car-recycling.service.ts (1 hunks)
- libs/application/templates/car-recycling/src/shared/types.ts (1 hunks)
- libs/clients/car-recycling/src/lib/carRecyclingClient.service.ts (2 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
apps/skilavottord/web/utils/dateUtils.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/skilavottord/ws/src/app/modules/vehicle/dto/createVehicle.input.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/application/template-api-modules/src/lib/modules/templates/car-recycling/car-recycling.service.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/application/templates/car-recycling/src/shared/types.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/clients/car-recycling/src/lib/carRecyclingClient.service.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."
🔇 Additional comments (6)
apps/skilavottord/ws/src/app/modules/vehicle/dto/createVehicle.input.ts (1)
17-18: LGTM: Nullable field aligns with PR objectivesThe change to make
firstRegistrationDatenullable aligns well with the PR objective of handling null values from the Samgöngustofa service. This modification improves the robustness of vehicle data handling as intended.A few points to note:
- The use of TypeScript decorators (
@Field) adheres to best practices for type safety in GraphQL schemas.- The nullable option
{ nullable: true }is correctly applied, allowing the field to be omitted when creating a vehicle input.- This change maintains type safety while adding flexibility, which is a good balance.
To ensure consistency, let's check if similar changes are needed in related files:
libs/application/templates/car-recycling/src/shared/types.ts (1)
9-9: Addition ofmodelYearproperty aligns with PR objectives and follows best practices.The new
modelYearproperty in theVehicleDtointerface is a good addition that addresses the PR objective of handling null values for model years from the Samgöngustofa service. This change enhances the robustness of vehicle data handling.Observations:
- The property is correctly typed as an optional
number, which allows for null or undefined values.- The change adheres to the coding guidelines for library files:
- It maintains reusability across different NextJS apps.
- It uses TypeScript for defining types.
- The optional nature of the property supports effective tree-shaking.
- The impact on existing codebase should be minimal due to the property being optional.
To ensure this change doesn't introduce any breaking changes, let's verify its usage:
✅ Verification successful
[/run_scripts]
Verified Addition of
modelYearPropertyThe addition of the
modelYearproperty to theVehicleDtointerface has been successfully verified across all usages. SincemodelYearis optional, it does not introduce any breaking changes to the existing codebase. All instances whereVehicleDtois used remain compatible with this addition.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usages of VehicleDto to ensure compatibility rg --type typescript "VehicleDto" -C 5Length of output: 73
Script:
#!/bin/bash # Search for usages of VehicleDto in TypeScript files to ensure compatibility rg "VehicleDto" --glob "*.ts" --glob "*.tsx" -C 5Length of output: 25043
libs/clients/car-recycling/src/lib/carRecyclingClient.service.ts (2)
75-76: Approved: Improved flexibility forfirstRegistrationDateparameterThe change from
DatetoDate | nullfor thefirstRegistrationDateparameter enhances the method's flexibility. This modification aligns well with the PR objective of handling null values for the model year, making the component more reusable across different scenarios where the registration date might not be available.
Line range hint
1-132: Overall assessment: Changes align with coding guidelines and improve service robustnessThe modifications to the
CarRecyclingClientServicealign well with the coding guidelines for thelibsdirectory:
Reusability: The changes enhance the flexibility of the
createVehiclemethod, making it more reusable across different NextJS apps that may or may not have the registration date available.TypeScript usage: The update to the
firstRegistrationDateparameter type (Date | null) demonstrates effective use of TypeScript for defining props and types.Tree-shaking and bundling: The changes don't introduce any new imports or exports, maintaining the current tree-shaking and bundling effectiveness.
The error handling improvements and the ability to handle null registration dates enhance the overall robustness of the service. These changes successfully address the PR objective of handling null values for the model year returned by the Samgöngustofa service.
libs/application/template-api-modules/src/lib/modules/templates/car-recycling/car-recycling.service.ts (2)
45-45: Initialization ofmodelYearis appropriateSetting
modelYeartonullensures that it has a default value before conditional assignment.
64-64: Verify thecreateVehiclemethod acceptsmodelYearcorrectlyPlease confirm that the
createVehiclemethod inCarRecyclingClientServiceacceptsmodelYearas a parameter and that the data type matches the method's expectations.Run the following script to verify the method signature:
...cation/template-api-modules/src/lib/modules/templates/car-recycling/car-recycling.service.ts
Outdated
Show resolved
Hide resolved
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16220 +/- ##
==========================================
- Coverage 36.96% 36.96% -0.01%
==========================================
Files 6779 6779
Lines 139875 139879 +4
Branches 39777 39778 +1
==========================================
Hits 51700 51700
- Misses 88175 88179 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Sentry.
|
Datadog ReportAll test runs ✅ 6 Total Test Services: 0 Failed, 6 Passed Test Services
🔻 Code Coverage Decreases vs Default Branch (1)
|
TS-921
What
Handle null value in modelYear
Why
Samgöngustofa service is returning null value for the modelYear when getting owners vehicles.
Checklist:
Summary by CodeRabbit
New Features
firstRegistrationDatefield when creating vehicle inputs, allowing for more flexible data entry.modelYearproperty to the vehicle data structure for enhanced vehicle recycling applications.Bug Fixes
Refactor