feat(application-system): On delete template api #16605
Conversation
WalkthroughThe pull request introduces several enhancements to the application deletion process within 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
libs/application/ui-components/src/components/ApplicationList/ApplicationList.tsx (1)
Line range hint 64-71: Add error handling to onApplicationDelete
The deletion callback should handle potential errors to ensure proper state management and user feedback through the newly added toast notifications.
- const onApplicationDelete = () => {
+ const onApplicationDelete = async () => {
+ try {
if ((applications.length - 1) % pageSize === 0 && page > 1) {
setPage(page - 1)
}
if (refetch) {
- refetch()
+ await refetch()
}
+ } catch (error) {
+ console.error('Failed to handle application deletion:', error)
+ // Assuming toast is from react-toastify
+ toast.error('Failed to update application list')
+ }
}libs/application/types/src/lib/StateMachine.ts (1)
134-134: Add JSDoc documentation for the onDelete property.
The new onDelete property looks good and follows the existing pattern of lifecycle hooks. However, consider adding JSDoc documentation to match other properties and improve maintainability.
+ /**
+ * Represents actions to be executed when an application is deleted.
+ * Can be either a single TemplateApi action or an array of actions.
+ */
onDelete?: TemplateApi<R>[] | TemplateApi<R>📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
- apps/application-system/api/src/app/modules/application/application.controller.ts (2 hunks)
- libs/application/core/src/lib/ApplicationTemplateHelper.ts (1 hunks)
- libs/application/types/src/lib/StateMachine.ts (1 hunks)
- libs/application/ui-components/src/components/ApplicationList/ApplicationList.tsx (2 hunks)
- libs/application/ui-components/src/hooks/useDeleteApplication.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
apps/application-system/api/src/app/modules/application/application.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."
libs/application/core/src/lib/ApplicationTemplateHelper.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/types/src/lib/StateMachine.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/ui-components/src/components/ApplicationList/ApplicationList.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."
libs/application/ui-components/src/hooks/useDeleteApplication.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 (5)
libs/application/ui-components/src/hooks/useDeleteApplication.ts (1)
3-4: LGTM! Good separation of concerns with error handling.
The addition of localization support and error handling utilities improves the hook's reusability across different NextJS apps while maintaining proper TypeScript typing.
Let's verify the error handling utility implementation:
Also applies to: 7-7
✅ Verification successful
Let me gather more information about the error handling implementation and its dependencies.
Error handling implementation is well-structured and properly integrated
The handleServerError utility follows good practices:
- Properly handles Apollo GraphQL errors using shared utilities (
findProblemInApolloError) - Integrates with the localization system via
FormatMessagetype - Uses standardized error handling from
@island.is/application/coreand@island.is/shared/problem - Provides user-friendly error messages through the UI toast system
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the handleServerError utility implementation
# Expected: The utility should handle GraphQL errors and support localization
# Check the implementation
rg -A 10 "export.*handleServerError" libs/application/ui-components/src/utilities/
Length of output: 1245
Script:
#!/bin/bash
# Check the findProblemInApolloError and getErrorReasonIfPresent implementations
rg -A 5 "findProblemInApolloError|getErrorReasonIfPresent" libs/application/ui-components/src/utilities/
# Check the types used in handleServerError
ast-grep --pattern 'type $_'
Length of output: 2313
libs/application/ui-components/src/components/ApplicationList/ApplicationList.tsx (1)
2-7: LGTM! Import changes follow best practices
The ToastContainer import is properly grouped with other UI components and maintains effective tree-shaking.
libs/application/types/src/lib/StateMachine.ts (1)
134-134: Verify the usage of onDelete property.
Let's verify how this property is being used in the application templates.
✅ Verification successful
The onDelete property is used for template API actions during application deletion
The property is correctly implemented in the codebase. It's used by ApplicationTemplateHelper to execute template-specific API actions when an application is being deleted. The implementation in application.controller.ts shows that these actions are properly handled and can be either a single action or an array of actions.
Key findings:
- Used in
ApplicationTemplateHelper.getOnDeleteStateAPIAction()to retrieve deletion-specific API actions - Properly handled in the application controller to support both single and multiple API actions
- Follows the same pattern as other template API actions (onEntry, onExit)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for usage of onDelete in application templates and state definitions
# Expected: Find implementations showing how onDelete is configured in templates
# Test 1: Look for direct usage of onDelete in state definitions
rg -t typescript "onDelete.*TemplateApi" -A 5
# Test 2: Look for state machine configurations that might use this property
ast-grep --pattern 'meta: {
$$$
onDelete: $_
$$$
}'
Length of output: 142
Script:
#!/bin/bash
# Let's try different search patterns to find onDelete usage
# Search for onDelete in TypeScript/JavaScript files
rg "onDelete" --type-add 'ts:*.{ts,tsx}' --type-add 'js:*.{js,jsx}' -t ts -t js -C 3
# Search for state machine configurations
rg "StateMachine" --type-add 'ts:*.{ts,tsx}' --type-add 'js:*.{js,jsx}' -t ts -t js -C 3
# Look for template definitions that might use onDelete
ast-grep --pattern 'const $_ = {
$$$
onDelete: $_,
$$$
}'
Length of output: 98192
Script:
#!/bin/bash
# Let's search for usage of onDelete in application templates and state machine configurations
# Look for state machine configurations with onDelete
ast-grep --pattern 'states: {
$$$
meta: {
$$$
onDelete: $_
$$$
}
$$$
}'
# Search for ApplicationTemplateHelper usage of onDelete
rg "getOnDeleteStateAPIAction" --type-add 'ts:*.{ts,tsx}' --type-add 'js:*.{js,jsx}' -t ts -t js -C 3
Length of output: 1630
libs/application/core/src/lib/ApplicationTemplateHelper.ts (1)
121-128: LGTM! The implementation follows established patterns.
The new getOnDeleteStateAPIAction method:
- Maintains consistency with existing state action retrieval methods
- Uses TypeScript effectively with proper return type definition
- Follows best practices for null handling using optional chaining and nullish coalescing
- Is properly placed in the core library for reuse across different NextJS apps
Let's verify the usage of this new method:
✅ Verification successful
Implementation follows established patterns and is correctly used
The getOnDeleteStateAPIAction method:
- Follows the same pattern as
getOnExitStateAPIActionandgetOnEntryStateAPIAction - Is properly used in the application controller for handling application deletion
- Correctly uses the shared
getTemplateAPIActionhelper method for consistent API action handling - Has proper TypeScript typing and null handling with optional chaining
The implementation is verified to be consistent with the codebase's patterns and is being used appropriately in the deletion flow.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent usage of onDelete actions across the codebase
# Expected: Find references to onDelete in state machine configurations and its usage
# Test 1: Look for state machine configurations defining onDelete actions
rg -A 3 "onDelete:\s*{"
# Test 2: Look for usages of getOnDeleteStateAPIAction
rg "getOnDeleteStateAPIAction"
Length of output: 350
Script:
#!/bin/bash
# Let's try a different approach to find onDelete configurations and usages
# Search for onDelete in state machine configurations
rg "onDelete" -A 3
# Search for template configurations with onDelete
fd "template.json" --exec rg "onDelete" {} -A 3 \;
# Look at the implementation in the controller to understand usage
rg -A 5 "getOnDeleteStateAPIAction" "apps/application-system/api/src/app/modules/application/application.controller.ts"
Length of output: 51186
Script:
#!/bin/bash
# Let's check for state machine configurations to understand onDelete usage
# Look for state machine configurations
ast-grep --pattern 'stateMachineConfig = {
$$$
}'
# Search for meta configurations with onDelete
rg -B 2 -A 2 "meta:\s*{[^}]*onDelete"
# Look at the template helper implementation
rg -B 5 -A 5 "getTemplateAPIAction" "libs/application/core/src/lib/ApplicationTemplateHelper.ts"
Length of output: 1480
apps/application-system/api/src/app/modules/application/application.controller.ts (1)
1082-1082: Addition of 'locale' parameter is appropriate and correctly implemented
The new parameter @CurrentLocale() locale: Locale is properly added to the delete method and utilized within the method.
libs/application/ui-components/src/components/ApplicationList/ApplicationList.tsx
Show resolved
Hide resolved
apps/application-system/api/src/app/modules/application/application.controller.ts
Outdated
Show resolved
Hide resolved
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16605 +/- ##
==========================================
- Coverage 36.79% 36.78% -0.01%
==========================================
Files 6855 6855
Lines 142285 142311 +26
Branches 40576 40590 +14
==========================================
- Hits 52348 52347 -1
- Misses 89937 89964 +27 Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Datadog ReportAll test runs ✅ 21 Total Test Services: 0 Failed, 20 Passed Test ServicesThis report shows up to 10 services
🔻 Code Coverage Decreases vs Default Branch (17)
|
obmagnusson
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
apps/application-system/api/src/app/modules/application/application.controller.ts (3)
1110-1146: Enhance error handling for delete actions
While the error handling is good, consider these improvements:
- Add timeout handling for external API calls
- Implement retry logic for transient failures
- Add more detailed error logging for debugging
const result =
deletingApplication.externalData[api.externalDataId || api.action]
+ if (!result) {
+ this.logger.error(
+ `No result found for action ${api.action} on ${JSON.stringify(
+ template.name,
+ )}`,
+ )
+ throw new TemplateApiError('Action execution failed', 500)
+ }
this.logger.debug(Line range hint 1147-1162: Implement transaction-like behavior for resource cleanup
The sequential deletion of resources could leave the system in an inconsistent state if one operation fails. Consider implementing a rollback mechanism or using a transaction-like pattern.
Consider implementing a cleanup service that:
- Tracks the status of each deletion step
- Implements retry logic for failed operations
- Provides rollback capabilities
- Logs detailed cleanup status
Would you like me to help create a new cleanup service implementation?
1163-1168: Enhance audit logging for deletion process
Consider adding more metadata to the audit log to track the complete deletion process:
this.auditService.audit({
auth: user,
action: 'delete',
resources: existingApplication.id,
- meta: { type: existingApplication.typeId },
+ meta: {
+ type: existingApplication.typeId,
+ deletedResources: {
+ charges: true,
+ payments: true,
+ attachments: true,
+ history: true
+ },
+ onDeleteActionsExecuted: onDeleteActions ? onDeleteActions.length : 0
+ },
})📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- apps/application-system/api/src/app/modules/application/application.controller.ts (3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
apps/application-system/api/src/app/modules/application/application.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."
🔇 Additional comments (1)
apps/application-system/api/src/app/modules/application/application.controller.ts (1)
1082-1082: LGTM: Locale parameter addition
The addition of the locale parameter enables proper internationalization support for delete actions and error messages.
apps/application-system/api/src/app/modules/application/application.controller.ts
Show resolved
Hide resolved
obmagnusson
added
automerge
Asana Task
What
Added an onDelete template Api handler for Application States.
Error toast for application delete
Added audit log for application delete
Why
There Is a need for applications to notify 3rd parties on their deletion.
Checklist:
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation