WEB-342: Advance Accounting rule can not be edited for classification type#2702
Conversation
WalkthroughEdits template to pass both item and index to edit(). Adds edit(record, index) and updateTableData(updatedData, index) to open a form dialog, map dialog responses to AccountingMappingDTO for specific form types, immutably replace a table row, and emit updated table data. Adjusts select value mappings and guards GL account access. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Component as AdvancedAccountingMappingRuleComponent
participant Dialog as FormDialog
participant Table as TableData
participant Parent as ParentEmitter
User->>Component: Click Edit(item, index)
Component->>Dialog: Open dialog with record
Dialog-->>Component: Return response
alt response requires mapping
Component->>Component: Map response → AccountingMappingDTO
else no mapping needed
Component->>Component: Use dialog response as-is
end
Component->>Table: updateTableData(updatedRow, index) (immutable replace)
Component->>Parent: Emit updated table data
Dialog-->>Component: Close
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/advanced-accounting-mapping-rule/advanced-accounting-mapping-rule.component.ts (1)
291-315: Fix default value access pattern for edit mode.The default values at lines 299 and 307 use incorrect property access. When editing, the
valuesparameter is anAccountingMappingDTOobject with nestedvalueandglAccountproperties, but the code accesses flat properties that don't exist on this structure. This inconsistency will cause the edit dialog to show incorrect pre-filled values.Apply this diff to align with the
AccountingMappingDTOstructure:new SelectBase({ controlName: 'chargeOffReasonCodeValueId', label: 'Charge-off reason', - value: values ? values.chargeOffReasonCodeValueId : reasonOptions[0].id, + value: values ? values.value.id : reasonOptions[0].id, options: { label: 'name', value: 'id', data: reasonOptions }, required: true, order: 1 }), new SelectBase({ controlName: 'expenseAccountId', label: 'Expense Account', - value: values ? values.expenseAccountId : this.expenseAccountData[0].id, + value: values ? values.glAccount.id : this.expenseAccountData[0].id, options: { label: 'name', value: 'id', data: this.expenseAccountData }, required: true, order: 2 })
🧹 Nitpick comments (3)
src/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/advanced-accounting-mapping-rule/advanced-accounting-mapping-rule.component.ts (1)
156-156: Strengthen type safety on method signature.The
recordparameter is typed asany, which bypasses TypeScript's type checking benefits.Apply this diff to improve type safety:
- edit(record: any, index: number) { + edit(record: AccountingMappingDTO, index: number) {src/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/advanced-accounting-mapping-rule/advanced-accounting-mapping-rule.component.html (2)
38-38: Add trackBy function for performance optimization.The
*ngFordirective should include atrackByfunction to optimize change detection, especially when the table data changes frequently.As per coding guidelines.
Add a trackBy function to the component:
trackByIndex(index: number, item: any): number { return index; }Then update the template:
- <tr mat-row *matRowDef="let row; columns: tableDisplayedColumns"></tr> + <tr mat-row *matRowDef="let row; columns: tableDisplayedColumns; trackBy: trackByIndex"></tr>
38-38: Add trackBy function for better performance.The table row iteration at line 38 (
*matRowDef="let row; columns: tableDisplayedColumns") doesn't use a trackBy function. This can cause unnecessary re-renders when the table data changes.As per coding guidelines: verify trackBy on *ngFor for Angular code.
Add a trackBy function in the component:
trackByIndex(index: number, item: any): number { return index; }Then update the template:
- <tr mat-row *matRowDef="let row; columns: tableDisplayedColumns"></tr> + <tr mat-row *matRowDef="let row; columns: tableDisplayedColumns; trackBy: trackByIndex"></tr>Alternatively, if items have unique identifiers, consider tracking by a stable property:
trackByValueId(index: number, item: AccountingMappingDTO): number { return item.value.id; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/advanced-accounting-mapping-rule/advanced-accounting-mapping-rule.component.html(1 hunks)src/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/advanced-accounting-mapping-rule/advanced-accounting-mapping-rule.component.ts(3 hunks)
👮 Files not reviewed due to content moderation or server errors (2)
- src/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/advanced-accounting-mapping-rule/advanced-accounting-mapping-rule.component.ts
- src/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/advanced-accounting-mapping-rule/advanced-accounting-mapping-rule.component.html
🧰 Additional context used
📓 Path-based instructions (1)
src/app/**
⚙️ CodeRabbit configuration file
src/app/**: For Angular code: verify component separation, trackBy on *ngFor,
strict type safety, and clean observable patterns.
Files:
src/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/advanced-accounting-mapping-rule/advanced-accounting-mapping-rule.component.tssrc/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/advanced-accounting-mapping-rule/advanced-accounting-mapping-rule.component.html
🧬 Code graph analysis (1)
src/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/advanced-accounting-mapping-rule/advanced-accounting-mapping-rule.component.ts (2)
src/app/products/loan-products/models/loan-product.model.ts (1)
AccountingMappingDTO(209-212)src/app/shared/form-dialog/formfield/model/select-base.ts (1)
SelectBase(19-27)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run Lint, Build and Deploy
🔇 Additional comments (11)
src/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/advanced-accounting-mapping-rule/advanced-accounting-mapping-rule.component.ts (8)
136-142: LGTM! Clean immutable update pattern.The method correctly implements an immutable row update using array spread and slice operations, which is important for Angular's change detection.
317-341: LGTM! Correct property access for AccountingMappingDTO.The updated default values correctly access the nested
value.idandglAccount.idproperties from theAccountingMappingDTOstructure, ensuring proper pre-population of form fields during edit operations.
136-142: LGTM! Clean immutable update pattern.The method correctly implements an immutable array update by creating a new array without mutating the existing state, which is excellent for Angular change detection.
136-142: LGTM! Immutable update pattern is correct.The method correctly implements an immutable update by creating a new array with slicing, which is good for Angular change detection.
160-174: Consider handling additional form types in the edit method.The edit method only handles
BuydownFeeClassificationToIncomeandCapitalizedIncomeClassificationToIncome, while the add method (lines 100-118) also handlesChargeOffReasonExpenseandWriteOffReasonToExpense. Should edit functionality be available for these form types as well?If these form types should also be editable, consider applying a similar pattern:
if ([ 'ChargeOffReasonExpense', 'WriteOffReasonToExpense' ].includes(this.formType)) { const updateData: AccountingMappingDTO = { value: this.getValueData(response.data.value.chargeOffReasonCodeValueId), glAccount: this.getGlAccountData(response.data.value.expenseAccountId) }; this.updateTableData(updateData, index); }Note: If the PR scope is intentionally limited to classification types only, this is acceptable.
333-333: Verify the glAccount property access is consistent.Similar to line 325, this change correctly uses the nested
glAccount.idproperty from theAccountingMappingDTOstructure.
291-315: Potential inconsistency in getChargeOffReasonExpenseFormfields.Line 299 uses
values.chargeOffReasonCodeValueIdwhenvaluesis provided. However, based on the pattern ingetClassificationIncomeFormfields(line 325) where nested properties are accessed (values.value.id), this might need similar treatment if the edit flow is extended to handleChargeOffReasonExpenseform type.Verify whether
ChargeOffReasonExpenserecords follow the sameAccountingMappingDTOstructure. If they do, line 299 should be:value: values ? values.value.id : reasonOptions[0].id,And line 307 should be:
value: values ? values.glAccount.id : this.expenseAccountData[0].id,Note: This is only relevant if the edit functionality is extended to support this form type.
325-325: Approve nested property access
The nested access values.value.id aligns with AccountingMappingDTO.value: CodeValue and edit(record,…) passes the correct DTO.src/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/advanced-accounting-mapping-rule/advanced-accounting-mapping-rule.component.html (3)
27-28: LGTM! Correct parameter passing for edit functionality.The updated edit button correctly passes both the item object and its index to the edit method, enabling proper pre-population of form fields with existing data.
27-28: LGTM! Edit button now correctly passes item data.The changes properly pass both the item object and its index to the
edit()method, enabling the edit functionality.
27-28: LGTM! Template changes correctly support the edit flow.The changes properly pass both the item object and its index to the edit method, enabling the edit functionality to access the record data.
alberto-art3ch
force-pushed
the
WEB-342/advance-accounting-rule-can-not-be-edited-for-classification-type
branch
from
2d152df to
10baa73
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/advanced-accounting-mapping-rule/advanced-accounting-mapping-rule.component.ts (1)
156-185: Add edit branches for remaining form types.The
edit()method still lacks support forPaymentFundSource,FeesIncome, andPenaltyIncomeform types, which are supported in theadd()method. Users can add these mapping rules but cannot edit them, creating an inconsistent user experience.This issue was previously flagged in past review comments. Please implement the missing branches following the same pattern as the existing ones.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/advanced-accounting-mapping-rule/advanced-accounting-mapping-rule.component.html(1 hunks)src/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/advanced-accounting-mapping-rule/advanced-accounting-mapping-rule.component.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/advanced-accounting-mapping-rule/advanced-accounting-mapping-rule.component.html
🧰 Additional context used
📓 Path-based instructions (1)
src/app/**
⚙️ CodeRabbit configuration file
src/app/**: For Angular code: verify component separation, trackBy on *ngFor,
strict type safety, and clean observable patterns.
Files:
src/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/advanced-accounting-mapping-rule/advanced-accounting-mapping-rule.component.ts
🧬 Code graph analysis (1)
src/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/advanced-accounting-mapping-rule/advanced-accounting-mapping-rule.component.ts (2)
src/app/products/loan-products/models/loan-product.model.ts (1)
AccountingMappingDTO(209-212)src/app/shared/form-dialog/formfield/model/select-base.ts (1)
SelectBase(19-27)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run Lint, Build and Deploy
🔇 Additional comments (3)
src/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/advanced-accounting-mapping-rule/advanced-accounting-mapping-rule.component.ts (3)
136-142: LGTM! Clean immutable update pattern.The implementation correctly creates a new array without mutating the original, which is good practice for change detection.
335-335: LGTM! Correctly accesses nested DTO property.The change from flat property access to
values.value.idcorrectly aligns with theAccountingMappingDTOstructure used intableData.However, note that
getChargeOffReasonExpenseFormfields(Lines 307, 317) still uses flat properties (chargeOffReasonCodeValueId,expenseAccountId) instead of nested properties (values.value.id,values.glAccount.id). This inconsistency was flagged in previous review comments. Please ensure that all form field builders follow the same pattern for consistency.
343-343: LGTM! Correctly accesses nested DTO property.The change from flat property access to
values.glAccount.idcorrectly aligns with theAccountingMappingDTOstructure used intableData.
alberto-art3ch
force-pushed
the
WEB-342/advance-accounting-rule-can-not-be-edited-for-classification-type
branch
from
10baa73 to
186f86b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/advanced-accounting-mapping-rule/advanced-accounting-mapping-rule.component.ts (1)
88-94: Data structure inconsistency in add() method.Line 90 accesses
item.chargeOffReasonCodeValueId, which assumes a flat structure. However, after the changes in this PR, tableData should contain AccountingMappingDTO objects with nested structure (item.value.id).Apply this diff to maintain consistency:
this.allowAddAccountingMapping = true; - this.tableData.forEach((item: any) => this.currentFormValues.push(item.chargeOffReasonCodeValueId)); + this.tableData.forEach((item: AccountingMappingDTO) => this.currentFormValues.push(item.value.id)); if (this.accountingMappingOptions.length == this.currentFormValues.length) {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/advanced-accounting-mapping-rule/advanced-accounting-mapping-rule.component.html(1 hunks)src/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/advanced-accounting-mapping-rule/advanced-accounting-mapping-rule.component.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/advanced-accounting-mapping-rule/advanced-accounting-mapping-rule.component.html
🧰 Additional context used
📓 Path-based instructions (1)
src/app/**
⚙️ CodeRabbit configuration file
src/app/**: For Angular code: verify component separation, trackBy on *ngFor,
strict type safety, and clean observable patterns.
Files:
src/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/advanced-accounting-mapping-rule/advanced-accounting-mapping-rule.component.ts
🧬 Code graph analysis (1)
src/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/advanced-accounting-mapping-rule/advanced-accounting-mapping-rule.component.ts (2)
src/app/products/loan-products/models/loan-product.model.ts (1)
AccountingMappingDTO(209-212)src/app/shared/form-dialog/formfield/model/select-base.ts (1)
SelectBase(19-27)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run Lint, Build and Deploy
🔇 Additional comments (2)
src/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/advanced-accounting-mapping-rule/advanced-accounting-mapping-rule.component.ts (2)
136-142: LGTM! Clean immutable update pattern.The method correctly creates a new array without mutating the original, following Angular best practices for change detection.
334-334: LGTM! Correct alignment with AccountingMappingDTO structure.The value mappings now correctly access nested properties (
values.value.idandvalues.glAccount.id), which aligns with the AccountingMappingDTO interface structure.Based on coding guidelines
Also applies to: 342-342
alberto-art3ch
force-pushed
the
WEB-342/advance-accounting-rule-can-not-be-edited-for-classification-type
branch
from
186f86b to
e744abe
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/advanced-accounting-mapping-rule/advanced-accounting-mapping-rule.component.html(1 hunks)src/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/advanced-accounting-mapping-rule/advanced-accounting-mapping-rule.component.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/advanced-accounting-mapping-rule/advanced-accounting-mapping-rule.component.html
🧰 Additional context used
📓 Path-based instructions (1)
src/app/**
⚙️ CodeRabbit configuration file
src/app/**: For Angular code: verify component separation, trackBy on *ngFor,
strict type safety, and clean observable patterns.
Files:
src/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/advanced-accounting-mapping-rule/advanced-accounting-mapping-rule.component.ts
🧬 Code graph analysis (1)
src/app/products/loan-products/loan-product-stepper/loan-product-accounting-step/advanced-accounting-mapping-rule/advanced-accounting-mapping-rule.component.ts (3)
src/app/products/loan-products/models/loan-product.model.ts (1)
AccountingMappingDTO(209-212)src/app/shared/form-dialog/formfield/model/select-base.ts (1)
SelectBase(19-27)src/app/shared/models/general.model.ts (1)
GLAccount(13-24)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run Lint, Build and Deploy
✅ Actions performedComments resolved. |
1 similar comment
✅ Actions performedComments resolved. |
2 participants
Description
Advance Accounting rule can not be edited for classification type in the Buy Down Fee and Capitalized Income due the Edit button has not functionality linked
Related issues and discussion
WEB-342
Screenshots
Screen.Recording.2025-10-07.at.5.37.57.p.m.mov
Checklist
Please make sure these boxes are checked before submitting your pull request - thanks!
If you have multiple commits please combine them into one commit by squashing them.
Read and understood the contribution guidelines at
web-app/.github/CONTRIBUTING.md.Summary by CodeRabbit
New Features
Bug Fixes