fix(auth-admin): Show relevant error messages #16180
Conversation
WalkthroughThe changes enhance error handling in the delegation process by modifying the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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
|
Datadog ReportAll test runs ✅ 68 Total Test Services: 0 Failed, 66 Passed Test ServicesThis report shows up to 10 services
🔻 Code Coverage Decreases vs Default Branch (2) |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16180 +/- ##
=======================================
Coverage 36.69% 36.70%
=======================================
Files 6778 6779 +1
Lines 139683 139692 +9
Branches 39723 39741 +18
=======================================
+ Hits 51259 51268 +9
Misses 88424 88424 Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Sentry.
|
![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 (2)
libs/portals/admin/delegation-admin/src/screens/CreateDelegation/CreateDelegation.action.ts (1)
97-102: Improved error handling in createDelegationActionThe new error handling logic using
findProblemInApolloErrorprovides more specific error information, aligning with the PR objective of showing relevant error messages. This enhancement will improve the user experience by providing more detailed feedback.Consider adding a fallback message for clarity:
errorMessage: problem?.detail || 'An unexpected error occurred',This ensures that users always receive some form of error message, even if
problem.detailis undefined.libs/portals/admin/delegation-admin/src/screens/CreateDelegation/CreateDelegation.tsx (1)
408-410: Approve changes with a minor suggestion for improvementThe changes effectively implement the PR objective of showing relevant error messages, improving the user experience by providing more specific feedback when available. This implementation aligns well with the coding guidelines for TypeScript usage and doesn't affect the component's reusability or tree-shaking capabilities.
To further improve code clarity and maintainability, consider using the nullish coalescing operator (??) instead of the logical OR (||). This would more explicitly handle cases where
actionData?.errorMessagemight be an empty string:message={actionData?.errorMessage ?? formatMessage(m.errorDefault)}This change ensures that the default message is used only when
actionData?.errorMessageis null or undefined, not when it's an empty string (which might be a valid error message in some cases).
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- libs/portals/admin/delegation-admin/src/screens/CreateDelegation/CreateDelegation.action.ts (3 hunks)
- libs/portals/admin/delegation-admin/src/screens/CreateDelegation/CreateDelegation.tsx (1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
libs/portals/admin/delegation-admin/src/screens/CreateDelegation/CreateDelegation.action.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/portals/admin/delegation-admin/src/screens/CreateDelegation/CreateDelegation.tsx (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 (3)
libs/portals/admin/delegation-admin/src/screens/CreateDelegation/CreateDelegation.action.ts (3)
14-17: Improved error handling with shared utilitiesThe addition of
findProblemInApolloErrorandProblemTypefrom the shared problem package aligns well with the PR objective of showing relevant error messages. This also adheres to the coding guideline of promoting reusability across different NextJS apps.
57-57: Enhanced error reporting in CreateDelegationResult typeThe addition of the optional
errorMessageproperty to theCreateDelegationResulttype enhances error reporting capabilities, aligning with the PR objective of showing relevant error messages. This change also adheres to the TypeScript usage guideline for defining types.
Line range hint
1-105: Summary: Effective implementation of error handling improvementsThe changes in this file successfully implement improved error handling and align well with the PR objectives and coding guidelines. The use of shared utilities, TypeScript types, and detailed error reporting enhances the overall quality and user experience of the application.
To ensure the effectiveness of these changes:
Please run the following script to check for consistent error handling across the codebase:
This will help identify any other locations where similar error handling improvements could be applied for consistency.
✅ Verification successful
It looks like the previous script encountered an error due to an unrecognized file type. Let's adjust the script to correctly target TypeScript files.
Verification Successful: Consistent Error Handling Confirmed
The use of
findProblemInApolloErrorwithin catch blocks has been consistently implemented across the codebase, including in the reviewed fileCreateDelegation.action.tsandPeriodsRepeater/index.tsx. No discrepancies were found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistent use of findProblemInApolloError in catch blocks # Test: Search for catch blocks and verify they use findProblemInApolloError rg --type typescript 'catch.*\{' -A 5 | rg 'findProblemInApolloError'Length of output: 104
Script:
#!/bin/bash # Description: Check for consistent use of findProblemInApolloError in catch blocks # Test: Search for catch blocks and verify they use findProblemInApolloError rg -g "*.ts" -g "*.tsx" 'catch\s*\(.*\)\s*\{' -A 5 | rg 'findProblemInApolloError'Length of output: 362
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (5)
libs/shared/utils/src/lib/errorCodes.ts (1)
1-8: LGTM! Consider adding documentation for each error code.The
ErrorCodesenum is well-structured and follows TypeScript best practices. It's properly exported and uses a consistent naming convention. To improve maintainability and assist library consumers, consider adding JSDoc comments explaining the purpose and usage of each error code.Example of how you could add documentation:
export enum ErrorCodes { /** Mismatch between national IDs in Zendesk */ ZENDESK_NATIONAL_IDS_MISMATCH = 'ZENDESK_NATIONAL_IDS_MISMATCH', /** Required custom fields are missing in Zendesk */ ZENDESK_CUSTOM_FIELDS_MISSING = 'ZENDESK_CUSTOM_FIELDS_MISSING', // ... (add comments for other error codes) }libs/portals/admin/delegation-admin/src/constants/errors.ts (1)
1-2: Consider using a more descriptive import name.The import
mfrom '../lib/messages' could be renamed to improve code readability. A more descriptive name likemessagesorerrorMessageswould make the code's intent clearer.Consider applying this change:
-import { m } from '../lib/messages' +import { m as errorMessages } from '../lib/messages'Then update all occurrences of
m.toerrorMessages.in the file.libs/auth-api-lib/src/lib/delegations/admin/delegation-admin-custom.service.ts (3)
63-66: Improved error handling: LGTM with a suggestionThe updated
BadRequestExceptionwith a structured error object is a good improvement. It provides more detailed information for error handling on the client side.Consider extracting the error message to a constant or using a localization key for easier maintenance and potential future localization:
const MISSING_CUSTOM_FIELDS_ERROR = 'Zendesk ticket is missing required custom fields'; throw new BadRequestException({ message: MISSING_CUSTOM_FIELDS_ERROR, error: ErrorCodes.ZENDESK_CUSTOM_FIELDS_MISSING });
163-168: Consistent error handling improvement: LGTM with a suggestionThe updated
BadRequestExceptioninstances with structured error objects are a good improvement. They provide more detailed and consistent error information.For better maintainability and consistency, consider extracting the error messages to constants or using localization keys, similar to the suggestion for the
getNationalIdsFromZendeskTicketmethod. This approach would make it easier to manage and potentially localize error messages in the future.Example:
const ZENDESK_ERRORS = { MISSING_TAG: 'Zendesk case is missing required tag', NOT_SOLVED: 'Zendesk case is not solved', NATIONAL_IDS_MISMATCH: 'National Ids do not match the Zendesk ticket' }; // Then use it like this: throw new BadRequestException({ message: ZENDESK_ERRORS.MISSING_TAG, error: ErrorCodes.ZENDESK_TAG_MISSING });Also applies to: 172-177, 188-191
275-278: Consistent error handling in validation: LGTM with a suggestionThe updated
BadRequestExceptioninstances in thevalidatePersonsNationalIdsmethod are consistent with the new error handling approach. They provide more detailed and specific error information.For consistency with the previous suggestions, consider extracting the error messages to constants or using localization keys. This would improve maintainability and make future localization easier if needed.
Example:
const VALIDATION_ERRORS = { SAME_NATIONAL_ID: 'National Ids cannot be the same', INVALID_PERSON: 'National Ids are not valid' }; // Usage: throw new BadRequestException({ message: VALIDATION_ERRORS.SAME_NATIONAL_ID, error: ErrorCodes.INPUT_VALIDATION_SAME_NATIONAL_ID });Also applies to: 286-289
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
- libs/auth-api-lib/src/lib/delegations/admin/delegation-admin-custom.service.ts (6 hunks)
- libs/portals/admin/delegation-admin/src/constants/errors.ts (1 hunks)
- libs/portals/admin/delegation-admin/src/lib/messages.ts (1 hunks)
- libs/portals/admin/delegation-admin/src/screens/CreateDelegation/CreateDelegation.action.ts (3 hunks)
- libs/portals/admin/delegation-admin/src/screens/CreateDelegation/CreateDelegation.tsx (2 hunks)
- libs/shared/utils/src/index.ts (1 hunks)
- libs/shared/utils/src/lib/errorCodes.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- libs/portals/admin/delegation-admin/src/screens/CreateDelegation/CreateDelegation.action.ts
- libs/portals/admin/delegation-admin/src/screens/CreateDelegation/CreateDelegation.tsx
🧰 Additional context used
📓 Path-based instructions (5)
libs/auth-api-lib/src/lib/delegations/admin/delegation-admin-custom.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/portals/admin/delegation-admin/src/constants/errors.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/portals/admin/delegation-admin/src/lib/messages.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/shared/utils/src/index.ts (2)
Pattern
libs/shared/**/*: "Confirm that the code adheres to the following:
- Cross-application utility functions, types, and constants.
- TypeScript best practices to ensure type safety and reusability.
- Documentation and examples for library consumers."
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/shared/utils/src/lib/errorCodes.ts (2)
Pattern
libs/shared/**/*: "Confirm that the code adheres to the following:
- Cross-application utility functions, types, and constants.
- TypeScript best practices to ensure type safety and reusability.
- Documentation and examples for library consumers."
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 (10)
libs/portals/admin/delegation-admin/src/constants/errors.ts (1)
4-11: LGTM! Well-structured error mapping.The
FORM_ERRORSconstant is well-implemented:
- It uses computed property names with the
ErrorCodesenum, ensuring type safety.- The mapping covers various error scenarios, providing specific messages for each case.
- The structure is consistent and easy to maintain.
This approach will make it easier to display relevant error messages in the create form, aligning with the PR's objective.
libs/shared/utils/src/index.ts (1)
26-26: New export added for error codes utility.The addition of
export * from './lib/errorCodes'aligns well with the existing structure of this index file, which exports various utility functions and types. This change enhances the shared utilities by including error codes, which can be beneficial for consistent error handling across the application.To ensure this addition meets our coding guidelines, let's verify the contents and structure of the
errorCodesmodule:This script will help us confirm that the
errorCodesmodule adheres to our coding guidelines, including TypeScript best practices, proper exports, and documentation for library consumers.libs/portals/admin/delegation-admin/src/lib/messages.ts (5)
124-127: LGTM: New error message for national ID mismatch.The
nationalIdsMismatchErrormessage is well-defined and follows the existing pattern in the file. The id is descriptive, and the default message clearly communicates the error condition.
136-139: LGTM: New error message for missing Zendesk custom fields.The
zendeskCustomFieldsMissingErrormessage is well-defined and follows the existing pattern in the file. The id is descriptive, and the default message clearly communicates the error condition.
140-143: LGTM: New error message for identical national IDs.The
sameNationalIdErrormessage is well-defined and follows the existing pattern in the file. The id is descriptive, and the default message clearly communicates the error condition.
144-147: LGTM: New error message for invalid national IDs.The
validPersonErrormessage is well-defined and follows the existing pattern in the file. The id is descriptive, and the default message clearly communicates the error condition.
124-147: Overall assessment: Good additions with minor improvements needed.The new error messages enhance the user experience by providing more specific feedback, which aligns with the PR objectives. The additions follow the existing patterns in the file and adhere to the coding guidelines for reusability and TypeScript usage.
Please address the typos mentioned in the previous comments. Once these are fixed, the changes will significantly improve error handling in the delegation admin interface.
Regarding the coding guidelines:
- Reusability: These message definitions can be easily imported and used across different NextJS apps within the project.
- TypeScript usage: The
defineMessagesfunction ensures type safety for the message definitions.- Tree-shaking and bundling: The use of named exports allows for effective tree-shaking in the build process.
libs/auth-api-lib/src/lib/delegations/admin/delegation-admin-custom.service.ts (3)
31-31: LGTM: Import of ErrorCodesThe addition of the
ErrorCodesimport from the shared utils package is appropriate for the new error handling approach. This adheres to the coding guidelines for reusability across different NextJS apps.
Line range hint
1-301: Overall structure and guideline adherence: LGTMThe file structure and the implemented changes adhere well to the coding guidelines:
- The code is part of the
libsdirectory and maintains reusability across different NextJS apps.- TypeScript is used effectively throughout the file for type definitions and interfaces.
- The changes focus on improving error handling without affecting tree-shaking or bundling practices.
The modifications enhance the error reporting mechanism while maintaining the overall structure and purpose of the service.
Line range hint
1-301: Summary of changes: Improved error handlingThe modifications in this file consistently enhance the error handling mechanism across the
DelegationAdminCustomService. Key improvements include:
- Introduction of structured error objects with specific error codes.
- Consistent use of
ErrorCodesfrom a shared utility package.- More detailed and informative error messages.
These changes will significantly improve error identification and handling on the client side, leading to better debugging and user experience. The code maintains good adherence to the project's coding guidelines for reusability, TypeScript usage, and effective tree-shaking.
To further enhance maintainability, consider implementing the suggested extractions of error messages to constants or localization keys.
saevarma
added
the
deprecated:automerge
* show propper error messages * chore: nx format:write update dirty files * return error message and code, use code to translate error on client * chore: nx format:write update dirty files * Fix typos * Explicitly type FORM_ERRORS --------- Co-authored-by: andes-it <builders@andes.is> Co-authored-by: Sævar Már Atlason <saevar.m.atlason@gmail.com> Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
What
Show relevant error messages in create form
Why
Help user fix errors
Screenshots / Gifs
Attach Screenshots / Gifs to help reviewers understand the scope of the pull request
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes