feat(transfer-of-machine-ownership): aosh ondelete #16784
Conversation
WalkthroughThis pull request introduces several changes related to the transfer of machine ownership functionality. A new TypeScript file is added for generating rejection emails, alongside modifications to the service handling application deletions and notifications. Additionally, a mockable payment catalog API is introduced, and updates are made to the state machine configuration. The OpenAPI specification for machine management is also revised to include a new endpoint for deleting owner change requests and other adjustments to existing endpoints. 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: 4
🧹 Outside diff range and nitpick comments (13)
libs/application/templates/aosh/transfer-of-machine-ownership/src/shared/constants.ts (1)
Line range hint
1-7: Consider adding JSDoc comments to document the enum.Adding JSDoc comments would improve IDE support and make the purpose of each action clearer to other developers.
+/** + * Actions available for machine ownership transfer API operations. + */ export enum ApiActions { + /** Initiates the review process for an application */ initReview = 'initReview', + /** Rejects an application with provided reason */ rejectApplication = 'rejectApplication', + /** Submits a new application for review */ submitApplication = 'submitApplication', + /** Deletes an application during the review process */ deleteApplication = 'deleteApplication', }libs/application/templates/aosh/transfer-of-machine-ownership/src/dataProviders/index.ts (1)
17-23: LGTM: Well-structured mock API configurationThe mock API configuration maintains parity with the real API, ensuring consistent behavior in test environments.
Consider adding a brief JSDoc comment to document the mock API's purpose and usage:
+/** + * Mockable version of VinnueftirlitidPaymentCatalogApi for testing purposes. + * Maintains the same configuration as the real API. + */ export const MockableVinnueftirlitidPaymentCatalogApi = MockablePaymentCatalogApi.configure({ params: {libs/application/template-api-modules/src/lib/modules/templates/aosh/transfer-of-machine-ownership/emailGenerators/applicationDeleteEmail.ts (4)
26-28: Enhance error messages with more context.The error messages should include more context to help with debugging. Consider including the application ID and other relevant information.
- if (!recipient.email) throw new Error('Recipient email was undefined') - if (!regNumber) throw new Error('Registration Number was undefined') - if (!rejectedBy?.ssn) throw new Error('Rejected by ssn was undefined') + if (!recipient.email) throw new Error(`Recipient email was undefined for application ${application.id}`) + if (!regNumber) throw new Error(`Registration Number was undefined for application ${application.id}`) + if (!rejectedBy?.ssn) throw new Error(`Rejected by ssn was undefined for application ${application.id}`)
30-30: Move email subject to constants file.For better maintainability and reusability, consider moving the email subject to a constants file that can be shared across the application.
// constants/email.ts export const EMAIL_SUBJECTS = { MACHINE_OWNERSHIP_TRANSFER_DELETED: 'Tilkynning um eigendaskipti - Umsókn afturkölluð' } as const;
39-78: Consider extracting email template to a separate file.The email template structure is quite large and could be moved to a separate template file for better organization and reusability.
Consider creating a dedicated templates directory with separate files for each email template. This would improve maintainability and make it easier to manage different email templates.
14-80: Consider breaking down the email generator function.The function is quite long and handles multiple responsibilities. Consider breaking it down into smaller, more focused functions for better maintainability and testing.
Example structure:
const validateEmailInputs = (application: Application, recipient: EmailRecipient, rejectedBy?: EmailRecipient) => { // validation logic } const buildEmailTemplate = (regNumber: string, application: Application, clientLocationOrigin: string) => { // template building logic } const generateApplicationRejectedEmail: ApplicationRejectedEmail = ( props, recipient, rejectedBy, ): Message => { const { application, options } = props const answers = application.answers as TransferOfMachineOwnershipAnswers validateEmailInputs(application, recipient, rejectedBy) return { from: buildEmailFrom(options.email), to: [buildEmailRecipient(recipient)], subject: EMAIL_SUBJECTS.MACHINE_OWNERSHIP_TRANSFER_DELETED, template: buildEmailTemplate(answers.machine.regNumber, application, options.clientLocationOrigin) } }libs/clients/work-machines/src/lib/workMachines.service.ts (1)
251-258: Consider enhancing error handling for deletion failuresThe implementation follows service patterns and correctly implements the deletion functionality. However, consider adding specific error handling for deletion failures, especially since this operation is part of a review process.
Consider wrapping the API call with error handling:
async deleteOwnerChange( auth: Auth, deleteChange: ApiMachineOwnerChangeOwnerchangeIdDeleteRequest, ) { - await this.machineOwnerChangeApiWithAuth( - auth, - ).apiMachineOwnerChangeOwnerchangeIdDelete(deleteChange) + try { + await this.machineOwnerChangeApiWithAuth( + auth, + ).apiMachineOwnerChangeOwnerchangeIdDelete(deleteChange) + } catch (error) { + // Re-throw with a more specific error message + throw new Error(`Failed to delete owner change: ${error.message}`) + } }libs/application/templates/aosh/transfer-of-machine-ownership/src/lib/TransferOfMachineOwnershipTemplate.ts (1)
219-221: LGTM: Clean implementation of delete handlerThe
onDeletelifecycle hook implementation aligns with the PR objectives for handling ownership deletion during review. Consider enhancing it with error handling for a more robust implementation.Consider wrapping the API call with error handling:
onDelete: defineTemplateApi({ action: ApiActions.deleteApplication, + onError: (error) => { + console.error('Failed to delete application:', error); + // Handle error appropriately + }, }),apps/application-system/api/infra/application-system-api.ts (1)
Line range hint
1-391: Well-structured service configuration following NextJS best practices.The service configuration demonstrates:
- Clear separation of concerns between worker and main service
- Proper environment-specific configurations
- Secure handling of sensitive information
- Comprehensive xroad service integration
Consider documenting the resource requirements and environment-specific configurations in the service's README to help future maintainers understand the infrastructure decisions.
libs/clients/work-machines/src/clientConfig.json (2)
Line range hint
259-288: Add error responses and documentation to the DELETE endpoint.The new DELETE endpoint for machine owner change should include:
- Common error responses (400, 401, 403, 404, 422) as implemented in other endpoints
- Description and summary fields for better API documentation
Apply this diff to improve the endpoint definition:
"/api/MachineOwnerChange/{OwnerchangeId}": { "delete": { "tags": ["MachineOwnerChange"], + "summary": "Delete a machine owner change request", + "description": "Deletes an existing machine owner change request identified by the OwnerchangeId", "parameters": [ { "name": "OwnerchangeId", "in": "path", "required": true, "schema": { "type": "string", "format": "uuid" } }, { "name": "X-Correlation-ID", "in": "header", "description": "Unique identifier associated with the request", "schema": { "type": "string", "format": "uuid" } } ], "responses": { "204": { "description": "No Content" }, + "400": { + "description": "Bad Request", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ValidationProblemDetails" + } + } + } + }, + "401": { + "description": "Unauthorized", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ProblemDetails" + } + } + } + }, + "403": { + "description": "Forbidden", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ProblemDetails" + } + } + } + }, + "404": { + "description": "Not Found", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ProblemDetails" + } + } + } + }, + "422": { + "description": "Unprocessable Content", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ProblemDetails" + } + } + } + } } } }
2187-2188: Add descriptions and validation constraints to new fields.The new fields
containerNumberandlocationOfMachineshould include:
- Field descriptions for better API documentation
- Validation constraints (e.g., maxLength) if applicable
Apply this diff to improve the field definitions:
"containerNumber": { "type": "string", + "description": "Container number for the machine", + "maxLength": 50, "nullable": true }, "locationOfMachine": { "type": "string", + "description": "Current location of the machine", + "maxLength": 255, "nullable": true },Also applies to: 2191-2192
charts/islandis/values.dev.yaml (2)
Line range hint
3686-3692: Consider optimizing resource allocationsThe web service has relatively high CPU limits (1000m) which could lead to node resource exhaustion in a development environment. Consider:
- Reducing CPU limits to 800m for better resource distribution
- Adjusting memory limits based on actual usage patterns
- Monitoring resource utilization to fine-tune these values
resources: limits: - cpu: '1000m' + cpu: '800m' memory: '768Mi' requests: cpu: '300m'
Line range hint
3647-3657: Consider adding documentation for service configurationsWhile the service configurations are well structured, they could benefit from additional documentation:
- Add comments explaining health check path purposes
- Document service dependencies and their requirements
- Include rationale for timeout and delay values
healthCheck: + # Kubernetes liveness probe to verify service is running liveness: initialDelaySeconds: 3 path: '/liveness' timeoutSeconds: 3 + # Kubernetes readiness probe to verify service can accept traffic readiness: initialDelaySeconds: 3 path: '/readiness' timeoutSeconds: 3
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (12)
apps/application-system/api/infra/application-system-api.ts(1 hunks)charts/islandis/values.dev.yaml(1 hunks)charts/islandis/values.prod.yaml(1 hunks)charts/islandis/values.staging.yaml(1 hunks)libs/application/template-api-modules/src/lib/modules/templates/aosh/transfer-of-machine-ownership/emailGenerators/applicationDeleteEmail.ts(1 hunks)libs/application/template-api-modules/src/lib/modules/templates/aosh/transfer-of-machine-ownership/transfer-of-machine-ownership.service.ts(2 hunks)libs/application/templates/aosh/transfer-of-machine-ownership/src/dataProviders/index.ts(2 hunks)libs/application/templates/aosh/transfer-of-machine-ownership/src/forms/TransferOfMachineOwnershipForm/prerequisitesSection.ts(2 hunks)libs/application/templates/aosh/transfer-of-machine-ownership/src/lib/TransferOfMachineOwnershipTemplate.ts(3 hunks)libs/application/templates/aosh/transfer-of-machine-ownership/src/shared/constants.ts(1 hunks)libs/clients/work-machines/src/clientConfig.json(4 hunks)libs/clients/work-machines/src/lib/workMachines.service.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (9)
apps/application-system/api/infra/application-system-api.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/aosh/transfer-of-machine-ownership/emailGenerators/applicationDeleteEmail.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/template-api-modules/src/lib/modules/templates/aosh/transfer-of-machine-ownership/transfer-of-machine-ownership.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/aosh/transfer-of-machine-ownership/src/dataProviders/index.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/aosh/transfer-of-machine-ownership/src/forms/TransferOfMachineOwnershipForm/prerequisitesSection.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/aosh/transfer-of-machine-ownership/src/lib/TransferOfMachineOwnershipTemplate.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/aosh/transfer-of-machine-ownership/src/shared/constants.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/work-machines/src/clientConfig.json (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/work-machines/src/lib/workMachines.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."
📓 Learnings (1)
libs/application/templates/aosh/transfer-of-machine-ownership/src/forms/TransferOfMachineOwnershipForm/prerequisitesSection.ts (1)
Learnt from: norda-gunni
PR: island-is/island.is#16471
File: libs/application/templates/driving-license/src/forms/prerequisites/sectionExternalData.ts:69-72
Timestamp: 2024-10-23T09:48:20.901Z
Learning: In `libs/application/templates/driving-license/src/forms/prerequisites/sectionExternalData.ts`, the `MockableSyslumadurPaymentCatalogApi` is not a replacement for `SyslumadurPaymentCatalogApi`; both providers should be included in the `dataProviders` array.
🔇 Additional comments (21)
libs/application/templates/aosh/transfer-of-machine-ownership/src/shared/constants.ts (1)
5-5: LGTM! The new enum member follows consistent naming.
The addition of deleteApplication to ApiActions aligns well with the feature requirements and maintains consistency with existing action names.
libs/application/templates/aosh/transfer-of-machine-ownership/src/dataProviders/index.ts (1)
4-4: LGTM: Clean import addition
The import follows TypeScript best practices and maintains consistency with existing imports.
libs/application/templates/aosh/transfer-of-machine-ownership/src/forms/TransferOfMachineOwnershipForm/prerequisitesSection.ts (2)
14-14: LGTM! Import follows established patterns.
The new MockableVinnueftirlitidPaymentCatalogApi import aligns with the existing import structure and naming conventions.
60-63: LGTM! Correct implementation of mock provider.
The mock provider is correctly added alongside the real VinnueftirlitidPaymentCatalogApi, following the established pattern of maintaining both providers. The empty title configuration matches the existing implementation.
Let's verify the mock provider implementation:
✅ Verification successful
Mock provider implementation is correctly configured
The verification confirms that:
- The mock provider (
MockableVinnueftirlitidPaymentCatalogApi) is properly configured with the same parameters as the real provider (VinnueftirlitidPaymentCatalogApi) - Both providers use the same organization ID (
VINNUEFTIRLITID) and external data ID (payment) - The mock provider is used alongside the real provider in the prerequisites section, following the established pattern
- The empty title configuration matches between both providers
The implementation follows the correct pattern for maintaining both real and mock providers in the application template.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the mock provider implementation and usage
# Expected: Find the mock provider class definition and its usage in tests
# Search for the mock provider implementation
echo "Searching for mock provider implementation..."
ast-grep --pattern 'class MockableVinnueftirlitidPaymentCatalogApi {
$$$
}'
# Search for test files using the mock provider
echo "Searching for test files using the mock provider..."
rg "MockableVinnueftirlitidPaymentCatalogApi" -g "*.spec.ts" -g "*.test.ts"
Length of output: 369
Script:
#!/bin/bash
# Let's try a broader search to find any references to this mock provider
echo "Searching for any references to MockableVinnueftirlitidPaymentCatalogApi..."
rg "MockableVinnueftirlitidPaymentCatalogApi" -A 5
# Let's also search for the real provider to understand the pattern
echo "Searching for the real VinnueftirlitidPaymentCatalogApi..."
rg "VinnueftirlitidPaymentCatalogApi" -A 5
# Let's check the directory where the current file is located
echo "Checking the directory structure..."
fd -p "libs/application/templates/aosh/transfer-of-machine-ownership" -t f
Length of output: 22202
Script:
#!/bin/bash
# Let's check the implementation of both the mock and real providers
echo "Checking the implementation of providers..."
rg -A 10 "export const VinnueftirlitidPaymentCatalogApi" libs/application/templates/aosh/transfer-of-machine-ownership/src/dataProviders/index.ts
echo "Checking the mock provider implementation..."
rg -A 10 "export const MockableVinnueftirlitidPaymentCatalogApi" libs/application/templates/aosh/transfer-of-machine-ownership/src/dataProviders/index.ts
# Let's also check for the base classes these providers extend from
echo "Checking for PaymentCatalogApi and MockablePaymentCatalogApi definitions..."
rg "class (PaymentCatalogApi|MockablePaymentCatalogApi)"
Length of output: 1401
libs/application/template-api-modules/src/lib/modules/templates/aosh/transfer-of-machine-ownership/emailGenerators/applicationDeleteEmail.ts (1)
1-13: LGTM! Well-structured imports and type definitions.
The TypeScript types are well-defined and the imports are properly organized. The ApplicationRejectedEmail type signature clearly defines the contract for the email generator function.
libs/clients/work-machines/src/lib/workMachines.service.ts (2)
5-5: LGTM: Import follows established patterns
The new import follows the existing pattern of importing types from the generated API client.
251-258: LGTM: Maintains service architecture and follows guidelines
The new functionality:
- Follows established service patterns
- Maintains proper TypeScript usage
- Preserves the reusable and modular nature of the library
libs/application/templates/aosh/transfer-of-machine-ownership/src/lib/TransferOfMachineOwnershipTemplate.ts (3)
33-33: LGTM: Clean import addition
The addition of MockableVinnueftirlitidPaymentCatalogApi follows TypeScript conventions and supports testing capabilities.
Line range hint 1-400: LGTM: Compliant with coding guidelines
The template implementation:
- ✅ Maintains reusability through modular structure
- ✅ Uses TypeScript effectively for type safety
- ✅ Follows proper bundling practices with dynamic imports
142-144: Verify the coexistence of mock and real APIs
Both MockableVinnueftirlitidPaymentCatalogApi and VinnueftirlitidPaymentCatalogApi are included in the API array. Please verify that this is intentional and that there's proper logic to determine which one to use.
✅ Verification successful
Based on the implementation details found, I can now provide a final response:
Both APIs are correctly configured for different purposes
The coexistence of both APIs is intentional and properly implemented:
VinnueftirlitidPaymentCatalogApiis configured with the real payment catalog APIMockableVinnueftirlitidPaymentCatalogApiis configured with the same parameters but uses a mockable version of the API- Both are configured with the same
externalDataId: 'payment'and organization ID, ensuring consistent behavior
This is a common pattern for testing and development purposes, where the mock API can be used in non-production environments while maintaining the same interface.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage patterns of both APIs to understand the selection logic
echo "Searching for API usage patterns..."
rg -A 5 "VinnueftirlitidPaymentCatalogApi|MockableVinnueftirlitidPaymentCatalogApi" --type typescript
# Search for environment-based configuration
echo "Searching for environment configuration..."
rg -A 5 "process\.env.*(_ENV|_MODE|NODE_ENV)" --type typescript
Length of output: 415
Script:
#!/bin/bash
# Search for API usage patterns without typescript file type restriction
echo "Searching for API usage patterns..."
rg -A 5 "VinnueftirlitidPaymentCatalogApi|MockableVinnueftirlitidPaymentCatalogApi"
# Search for environment configuration without typescript file type restriction
echo "Searching for environment configuration..."
rg -A 5 "process\.env.*(_ENV|_MODE|NODE_ENV)"
# Find all files containing these APIs
echo "Finding relevant files..."
fd -g "*.ts" -x grep -l "VinnueftirlitidPaymentCatalogApi"
Length of output: 71501
Script:
#!/bin/bash
# Search for the implementation of both APIs to understand their usage pattern
echo "Searching for API implementations..."
ast-grep --pattern 'export const $API = $$$' ./libs/application/templates/aosh/transfer-of-machine-ownership/src/dataProviders/index.ts
# Search for where these APIs are used together
echo "Searching for usage patterns..."
rg -B 5 -A 5 "buildDataProviderItem.*provider: (MockableVinnueftirlitidPaymentCatalogApi|VinnueftirlitidPaymentCatalogApi)"
Length of output: 2494
libs/application/template-api-modules/src/lib/modules/templates/aosh/transfer-of-machine-ownership/transfer-of-machine-ownership.service.ts (2)
355-361: LGTM! The owner change deletion logic is correctly implemented.
The added code properly handles the deletion of owner change in work machines service.
273-343: TypeScript usage follows best practices.
The code effectively uses TypeScript for:
- Method parameter types with
TemplateApiModuleActionProps - Return type annotations
- Type assertions for application answers
apps/application-system/api/infra/application-system-api.ts (2)
342-343: LGTM! Verify secret availability across environments.
The addition of the National Registry B2C client secret follows the established pattern for sensitive information management.
✅ Verification successful
Secret configuration verified across environments and services
The secret NATIONAL_REGISTRY_B2C_CLIENT_SECRET is properly configured:
- Present in all environments (dev, staging, prod) in the Helm charts
- Correctly referenced in services that use NationalRegistryB2C:
- User Notification Service
- User Profile Service
- Regulations Admin Backend
- Endorsement System API
- Application System API
- Main API
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the secret exists in the k8s configuration
# and NationalRegistryB2C service is properly configured
# Check if the secret path exists in other configuration files
echo "Checking for secret configuration..."
rg "NATIONAL_REGISTRY_B2C_CLIENT_SECRET" --type yaml
# Verify NationalRegistryB2C service usage
echo "Checking NationalRegistryB2C service usage..."
rg "NationalRegistryB2C" -A 5 -B 5
Length of output: 15373
Line range hint 383-386: Verify resource adjustment rationale.
The memory limits and requests have been increased significantly. While the ratio between limits and requests remains balanced, we should ensure these changes are backed by performance metrics.
✅ Verification successful
Let me generate additional scripts to specifically look for performance metrics or issues related to the application system API service.
Resource adjustments appear justified based on historical changes
The git history shows this is an intentional increase from previous values (768Mi to 1024Mi for limits, 384Mi to 512Mi for requests), maintaining a healthy ratio between requests and limits. The absence of performance issues in the codebase and the gradual scaling pattern suggests this is a proactive optimization rather than a reactive fix.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Look for performance-related discussions or issues
# that might justify the resource increases
# Search for related performance discussions
echo "Checking for performance-related discussions..."
rg -i "memory|performance|resource|oom|out.?of.?memory" --type md
# Look for potential memory-related configurations
echo "Checking for memory configurations..."
rg -i "memory:|memoryLimit|heap" --type yaml --type json
Length of output: 30663
Script:
#!/bin/bash
# Look for performance-related issues or discussions specific to application-system-api
echo "Checking for application-system-api performance issues..."
rg -i "application.?system.?api.*memory|application.?system.?api.*performance|application.?system.?api.*resource" --type md --type ts
# Look for memory configurations specific to application-system-api
echo "Checking application-system-api memory configurations..."
rg -i "memory.*application.?system.?api|application.?system.?api.*memory" --type yaml
# Check git history for memory-related changes in this file
echo "Checking git history for memory changes..."
git log -p -- apps/application-system/api/infra/application-system-api.ts | grep -i "memory"
Length of output: 863
libs/clients/work-machines/src/clientConfig.json (2)
290-302: LGTM!
The addition of X-Correlation-ID header parameter is consistent with other endpoints and properly documented.
2588-2590: LGTM!
The addition of the nameEn field follows the established pattern of providing English translations for names.
charts/islandis/values.staging.yaml (3)
796-796: Secret configuration looks good!
The NATIONAL_REGISTRY_B2C_CLIENT_SECRET configuration follows the established pattern and security best practices for secret management.
Line range hint 1479-1483: Security configurations are well implemented!
The security settings follow Kubernetes best practices:
- Proper securityContext with privilege escalation disabled
- Non-privileged containers
- Well-defined RBAC with specific service accounts
Line range hint 1468-1478: Verify resource allocation and scaling parameters
The web service has significantly high resource limits and aggressive horizontal scaling:
- CPU limit of 1000m is quite high
- Maximum 50 replicas could potentially cause resource pressure
Please verify if these values are necessary based on actual load patterns and resource utilization metrics.
charts/islandis/values.dev.yaml (2)
Line range hint 1-24: Security configurations follow Kubernetes best practices
The security configurations across services demonstrate proper implementation of security best practices:
- Privilege escalation is disabled
- Containers run as non-root
- Proper security groups are configured
Also applies to: 799-799
799-799: Environment configuration follows security best practices
The environment variables and secrets management demonstrates proper implementation:
- Sensitive data is stored in Kubernetes secrets
- Environment-specific configurations are properly separated
- Common configurations are shared appropriately across services
...dules/templates/aosh/transfer-of-machine-ownership/emailGenerators/applicationDeleteEmail.ts
Show resolved
Hide resolved
...odules/templates/aosh/transfer-of-machine-ownership/transfer-of-machine-ownership.service.ts
Outdated
Show resolved
Hide resolved
...odules/templates/aosh/transfer-of-machine-ownership/transfer-of-machine-ownership.service.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
libs/application/template-api-modules/src/lib/modules/templates/aosh/transfer-of-machine-ownership/transfer-of-machine-ownership.service.ts (1)
273-276: Add return type annotation for consistency.The method signature should include the return type annotation for better type safety and documentation.
- async deleteApplication({ + async deleteApplication({ application, auth, - }: TemplateApiModuleActionProps): Promise<void> { + }: TemplateApiModuleActionProps): Promise<void> {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
libs/application/template-api-modules/src/lib/modules/templates/aosh/transfer-of-machine-ownership/transfer-of-machine-ownership.service.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
libs/application/template-api-modules/src/lib/modules/templates/aosh/transfer-of-machine-ownership/transfer-of-machine-ownership.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."
...odules/templates/aosh/transfer-of-machine-ownership/transfer-of-machine-ownership.service.ts
Outdated
Show resolved
Hide resolved
...odules/templates/aosh/transfer-of-machine-ownership/transfer-of-machine-ownership.service.ts
Outdated
Show resolved
Hide resolved
...odules/templates/aosh/transfer-of-machine-ownership/transfer-of-machine-ownership.service.ts
Outdated
Show resolved
Hide resolved
Datadog ReportAll test runs ✅ 5 Total Test Services: 0 Failed, 5 Passed Test Services
🔻 Code Coverage Decreases vs Default Branch (2) |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
libs/application/template-api-modules/src/lib/modules/templates/aosh/transfer-of-machine-ownership/transfer-of-machine-ownership.service.ts (2)
285-351: Implement transaction-like behavior for atomic operations.The method performs multiple dependent operations (charge deletion, owner change deletion, notifications) that should be handled atomically to maintain data consistency.
Consider implementing a transaction-like pattern:
- Create a transaction context to track completed operations
- Implement compensation logic for rollback
- Execute operations in order
- Roll back completed operations if any step fails
Would you like me to provide a detailed implementation example?
363-364: Consider further code deduplication with deleteApplication.While using
deleteOwnerChangereduces duplication, there's still significant overlap betweenrejectApplicationanddeleteApplication. Consider extracting the common workflow into a shared private method.+private async handleApplicationTermination( + application: Application, + auth: User, + terminationType: 'delete' | 'reject' +): Promise<void> { + // 1. Handle charge deletion + const chargeId = getPaymentIdFromExternalData(application) + if (chargeId) { + await this.chargeFjsV2ClientService.deleteCharge(chargeId) + } + + // 2. Handle owner change deletion + await this.deleteOwnerChange(auth, application.id) + + // 3. Handle notifications + const answers = application.answers as TransferOfMachineOwnershipAnswers + const recipientList = getRecipients(answers, [ + EmailRole.seller, + EmailRole.buyer, + ]) + const actionByRecipient = getRecipientBySsn( + answers, + auth.nationalId, + ) + await this.notifyRecipients(application, recipientList, actionByRecipient) +}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
libs/application/template-api-modules/src/lib/modules/templates/aosh/transfer-of-machine-ownership/transfer-of-machine-ownership.service.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
libs/application/template-api-modules/src/lib/modules/templates/aosh/transfer-of-machine-ownership/transfer-of-machine-ownership.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 (1)
libs/application/template-api-modules/src/lib/modules/templates/aosh/transfer-of-machine-ownership/transfer-of-machine-ownership.service.ts (1)
33-33: LGTM!
The import of the User type is appropriate for the new functionality.
...odules/templates/aosh/transfer-of-machine-ownership/transfer-of-machine-ownership.service.ts
Outdated
Show resolved
Hide resolved
...odules/templates/aosh/transfer-of-machine-ownership/transfer-of-machine-ownership.service.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
libs/application/template-api-modules/src/lib/modules/templates/aosh/transfer-of-machine-ownership/transfer-of-machine-ownership.service.ts (1)
293-367: Consider implementing transaction-like error handling.While individual operations have error handling, the method could benefit from a transaction-like approach to maintain system consistency if one operation fails.
async deleteApplication({ application, auth, }: TemplateApiModuleActionProps): Promise<void> { + const compensationActions: (() => Promise<void>)[] = []; + try { // 1. Delete charge const chargeId = getPaymentIdFromExternalData(application) try { if (chargeId) { await this.chargeFjsV2ClientService.deleteCharge(chargeId) + compensationActions.push(async () => { + // TODO: Implement charge restoration logic + }); } } catch (error) { this.logger.error( `Failed to delete charge ${chargeId} for application ${application.id}`, error, ) throw error } // 2. Delete owner change await this.deleteOwnerChange(auth, application.id) + compensationActions.push(async () => { + // TODO: Implement owner change restoration logic + }); // 3. Send notifications + } catch (error) { + this.logger.error( + `Failed to delete application ${application.id}`, + error, + ); + // Execute compensation actions in reverse order + for (const action of compensationActions.reverse()) { + try { + await action(); + } catch (compensationError) { + this.logger.error( + `Compensation action failed for application ${application.id}`, + compensationError, + ); + } + } + throw error; + } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
libs/application/template-api-modules/src/lib/modules/templates/aosh/transfer-of-machine-ownership/transfer-of-machine-ownership.service.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
libs/application/template-api-modules/src/lib/modules/templates/aosh/transfer-of-machine-ownership/transfer-of-machine-ownership.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 (4)
libs/application/template-api-modules/src/lib/modules/templates/aosh/transfer-of-machine-ownership/transfer-of-machine-ownership.service.ts (4)
33-33: LGTM!
The User type import is correctly added and necessary for the new functionality.
274-291: LGTM! Well-structured error handling.
The method effectively encapsulates the owner change deletion logic with proper error handling and logging.
293-367: LGTM! Well-structured implementation.
The method effectively handles the complete application deletion process including charge deletion, owner change deletion, and notifications.
379-381: LGTM! Consistent with deleteApplication implementation.
The changes properly utilize the extracted deleteOwnerChange method and maintain consistency with the deleteApplication implementation.
* adding delete functionality to ownerchange machine * fix: init * chore: charts update dirty files * adding mock payment * fix: add secret from aws paramter store * removing console.log * revome other pull request * remove console.log * adding delete async func * error handling --------- Co-authored-by: Þorkell Máni Þorkelsson <[email protected]> Co-authored-by: andes-it <[email protected]> Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
On delete
Attach a link to issue if relevant
What
Adding on delete functionality if owner deletes while in review
Why
Specify why you need to achieve this
Screenshots / Gifs
Attach Screenshots / Gifs to help reviewers understand the scope of the pull request
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores