WEB-657: Loan view and Repayment for Working Capital#3622
Open
alberto-art3ch wants to merge 1 commit into
Open
WEB-657: Loan view and Repayment for Working Capital#3622alberto-art3ch wants to merge 1 commit into
alberto-art3ch wants to merge 1 commit into
Conversation
|
Note
|
| Layer / File(s) | Summary |
|---|---|
Base resolver and service infrastructure src/app/loans/common-resolvers/loans-account-transaction.resolver.ts, src/app/loans/common-resolvers/loan-base.resolver.ts, src/app/loans/loans.service.ts |
LoansAccountTransactionResolver now extends LoanBaseResolver, calls initialize(route), validates route params, and calls getLoansAccountTransaction with a product-type discriminator; LoanBaseResolver.loanAccountPath return type tightened; LoansService adds applyWorkingCapitalLoanActionCommand() and updates getLoansAccountTransaction() to accept productType. |
Loan Action Template Routing src/app/loans/common-resolvers/loan-action-button.resolver.ts |
Make Repayment template resolution now selects loan-product or working-capital template based on loanProductService.isLoanProduct. |
Shared Account Header Component src/app/shared/account-header/* |
Adds AccountHeaderComponent (template, styles, TS) providing themed header projection with status indicator inputs and used by loans view. |
Loan Summary Balance Component src/app/loans/loans-view/general-tab/loan-summary-balance-component/* |
New LoanSummaryBalanceComponentComponent renders loan-summary mat-table; supports distinct column sets/rows for loan vs working-capital and optional Adjustments when chargebacks exist. |
General Tab Component and Template src/app/loans/loans-view/general-tab/general-tab.component.* |
GeneralTab delegates summary rendering to new component, makes currencyCode nullable, adds hasChargeBack, and conditionally renders repayment/maturity cells per product type. |
Loans View Header and Navigation src/app/loans/loans-view/loans-view.component.* |
Replaces inline header with mifosx-account-header projection, updates imports and styles, conditionally shows Dashboard tab, and adjusts Charges/tab labels. |
Make Repayment Form and Submission src/app/loans/loans-view/loan-account-actions/make-repayment/* |
Switches to typed reactive forms, uses DestroyRef for subscriptions, loads/waives penalties only for loan products, conditions waive UI on product/penalties, warns on waiver failure, and routes submission to loan or working-capital endpoints. |
Transactions Tab UI Restructuring src/app/loans/loans-view/transactions-tab/* |
Replaces checkbox toolbar with slide-toggles, populates displayedColumns at runtime by product type, conditions amount/interest/balance cells, updates journal-entry route, adds row border class helper, and overhauls SCSS for badges and row-type color variants. |
Transaction Detail View src/app/loans/loans-view/transactions/view-transaction/* |
Treats transactionType as nullable, copies transactionDate for working-capital, shows reversed transaction details, updates undo eligibility to consider both reversed and manuallyReversed, and includes productType in back navigation. |
Export Dialog and Minor Refinements src/app/loans/loans-view/transactions/export-transactions/* |
Adjusts export dialog spacing, button padding, and form height; removes trailing in the generate button label. |
Working Capital Schedule and Balance Tabs src/app/loans/loans-view/working-capital/* |
Fixes Discount Factor formatting precision and refactors loan-balances tab to use DestroyRef/takeUntilDestroyed in ngOnInit. |
Theme Styling and UI Polish src/app/shared/notifications-tray/*.scss, src/app/shared/theme-toggle/*.scss, src/app/loans/loans-view/loans-view.component.scss |
Adds theme-aware header projection coloring and refines icon/button transitions/hover states. |
Localization src/assets/translations/*.json |
Adds "User Profile" translation key across many locales and corrects Disbursement capitalization in es-MX. |
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
- openMF/web-app#2714: touches LoanActionButtonResolver (similar action-template changes).
- openMF/web-app#3419: similar working-capital action template branching for other actions.
- openMF/web-app#3100: related Make Repayment UX changes.
Suggested reviewers
- adamsaghy
- IOhacker
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title clearly and concisely summarizes the main objective: adding loan view and repayment functionality for Working Capital products, which is the primary focus of this comprehensive changeset. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
| Linked Issues check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands and usage tips.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
🧹 Nitpick comments (7)
src/app/shared/theme-toggle/theme-toggle.component.scss (3)
11-17: ⚡ Quick winLeverage SCSS theme variables instead of hardcoded colors.
Similar to the notifications-tray component, this styling uses hardcoded color values (
rgb(255 255 255 / 72%),rgb(255 255 255 / 15%),#fff) instead of SCSS theme variables. As per coding guidelines, leverage SCSS variables fromsrc/main.scssandsrc/theme/mifosx-theme.scssrather than explicit color values.♻️ Suggested refactor using theme variables
:host button { - color: rgb(255 255 255 / 72%); + color: rgba($white, 0.72); border-radius: 50%; transition: background-color 0.2s ease, color 0.2s ease; &:hover { - background-color: rgb(255 255 255 / 15%); - color: `#fff`; + background-color: rgba($white, 0.15); + color: $white; } }Note: You'll need to import the color variables at the top of the file:
`@use` '../../../assets/styles/colours' as *;As per coding guidelines: Leverage SCSS variables defined in
src/main.scssandsrc/theme/mifosx-theme.scssrather than generating custom classes and explicit pixel values.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/shared/theme-toggle/theme-toggle.component.scss` around lines 11 - 17, The SCSS in theme-toggle.component.scss uses hardcoded colors (e.g., rgb(255 255 255 / 72%), rgb(255 255 255 / 15%), `#fff`); update the styles in this file to use the project SCSS theme variables instead: import the colour variables (e.g., add `@use` '../../../assets/styles/colours' as *; at the top) and replace the hardcoded values in the theme-toggle selector and its &:hover rules with the corresponding variables from the colours/mifosx-theme files so the component follows the shared theme.
9-9: 💤 Low valueConsider using more specific MDC-based selector for consistency.
While
:host buttonworks, using:host button.mat-mdc-icon-buttonwould be more specific and align with the MDC-based Material selector pattern used innotifications-tray.component.scss. Based on learnings, MDC DOM classes should be preferred when styling Angular Material v20+ components.♻️ Suggested selector update
-:host button { +:host button.mat-mdc-icon-button { color: rgb(255 255 255 / 72%);Based on learnings: In Angular Material v20+ (MDC-based), prefer styling MDC DOM classes (e.g.,
.mat-mdc-icon-button) in SCSS instead of legacy selectors.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/shared/theme-toggle/theme-toggle.component.scss` at line 9, The selector in theme-toggle.component.scss is currently using the generic host rule `:host button`; replace it with the MDC-specific selector `:host button.mat-mdc-icon-button` so styles target the Angular Material v20+ MDC icon button class consistently (update any related nested rules or specificity to match the new selector).
9-20: ⚖️ Poor tradeoffCode duplication with notifications-tray component.
This styling block is nearly identical to the one added in
notifications-tray.component.scss(lines 24-35), with only a slight difference in base color opacity (72% vs 80%). Consider extracting this common icon button hover pattern into a shared SCSS mixin to maintain consistency and reduce duplication.♻️ Example shared mixin approach
In a shared styles file (e.g.,
src/assets/styles/mixins.scss):`@mixin` icon-button-hover($base-opacity: 0.8) { color: rgba($white, $base-opacity); border-radius: 50%; transition: background-color 0.2s ease, color 0.2s ease; &:hover { background-color: rgba($white, 0.15); color: $white; } }Then in both component files:
`@use` '../../../assets/styles/mixins' as *; :host button.mat-mdc-icon-button { `@include` icon-button-hover(0.8); // or 0.72 for theme-toggle }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/shared/theme-toggle/theme-toggle.component.scss` around lines 9 - 20, Extract the duplicated icon-button styles from theme-toggle.component.scss and notifications-tray.component.scss into a shared SCSS mixin (e.g., icon-button-hover) in a common mixins.scss, parameterize the base color opacity (default and override), and replace the duplicated blocks in both components by importing the mixins file and including the mixin (use the mixin name icon-button-hover with the appropriate opacity argument for ThemeToggle and NotificationsTray); update selectors to match existing button selector (e.g., :host button or :host button.mat-mdc-icon-button) so behavior and hover states remain identical.src/app/shared/notifications-tray/notifications-tray.component.scss (1)
24-35: ⚡ Quick winLeverage SCSS theme variables instead of hardcoded colors.
This block uses hardcoded color values (
rgb(255 255 255 / 80%),rgb(255 255 255 / 15%),#fff) instead of the SCSS variables defined in theme files. The rest of this file consistently uses variables like$white,$asbestos, etc. As per coding guidelines, SCSS variables fromsrc/main.scssandsrc/theme/mifosx-theme.scssshould be leveraged rather than explicit color values.♻️ Suggested refactor using theme variables
:host button.mat-mdc-icon-button { - color: rgb(255 255 255 / 80%); + color: rgba($white, 0.8); border-radius: 50%; transition: background-color 0.2s ease, color 0.2s ease; &:hover { - background-color: rgb(255 255 255 / 15%); - color: `#fff`; + background-color: rgba($white, 0.15); + color: $white; } }As per coding guidelines: Leverage SCSS variables defined in
src/main.scssandsrc/theme/mifosx-theme.scssrather than generating custom classes and explicit pixel values.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/shared/notifications-tray/notifications-tray.component.scss` around lines 24 - 35, The :host button.mat-mdc-icon-button block uses hardcoded colors (rgb(...) and `#fff`); update it to use the theme SCSS variables (e.g. $white) and alpha via rgba() or existing opacity helper variables from your theme files: replace rgb(255 255 255 / 80%) with rgba($white, 0.8), rgb(255 255 255 / 15%) with rgba($white, 0.15), and `#fff` with $white in the :host button.mat-mdc-icon-button and its &:hover rule so the component follows the project's theme variables defined in src/main.scss and src/theme/mifosx-theme.scss.src/app/loans/loans-view/transactions/export-transactions/export-transactions.component.scss (2)
9-11: 💤 Low valueAdjust height to follow the 8px grid system.
The
170pxheight doesn't align with the 8px grid system (170 ÷ 8 = 21.25). Consider using168px(21 × 8) or176px(22 × 8) instead to maintain visual consistency across the application.As per coding guidelines: Stick to the 8px grid system for visual design and spacing.
📐 Proposed fix to align with 8px grid
form { - height: 170px; + height: 168px; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/loans/loans-view/transactions/export-transactions/export-transactions.component.scss` around lines 9 - 11, The form selector in export-transactions.component.scss uses height: 170px which breaks the 8px grid; change the height value in the form rule (in export-transactions.component.scss) to an 8px-multiple such as 168px (21×8) or 176px (22×8) to restore grid alignment and visual consistency across the app.
15-15: ⚖️ Poor tradeoffConsider using SCSS variables instead of hardcoded percentages.
The padding values use hardcoded percentages rather than SCSS variables from the theme files. While functional, this approach makes it harder to maintain consistent spacing across the application and adapt to theme changes.
As per coding guidelines: Leverage SCSS variables defined in
src/main.scssandsrc/theme/mifosx-theme.scssrather than generating custom classes and explicit pixel values.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/loans/loans-view/transactions/export-transactions/export-transactions.component.scss` at line 15, The rule uses hardcoded padding percentages in export-transactions.component.scss (padding: 1% 0 2%); replace those literal values with the project's SCSS spacing variables (from src/main.scss or src/theme/mifosx-theme.scss) to ensure consistent theming—e.g., map 1%/2% to the appropriate $spacing-* variables, import the theme file at the top of export-transactions.component.scss if not already imported, and update the selector (export-transactions component styles) to use the variables instead of literal percentages.src/app/loans/loans-view/working-capital/loan-balances-tab/loan-balances-tab.component.ts (1)
36-36: ⚡ Quick winConsider introducing a typed interface for the route data.
The
loanDetailsData: anyparameter weakens type safety. While the retrieved learning notes that typing the full API response layer can be tracked as a separate cross-cutting refactor, introducing a simple interface here would provide immediate type safety benefits with minimal effort.🎯 Suggested typing improvement
Add an interface near the top of the file:
interface LoanBalancesRouteData { loanDetailsData: { currency: { code: string }; balance: WorkingCapitalBalances; }; }Then update the subscription:
- this.route.parent.data.pipe(takeUntilDestroyed(this.destroyRef)).subscribe((data: { loanDetailsData: any }) => { + this.route.parent.data.pipe(takeUntilDestroyed(this.destroyRef)).subscribe((data: LoanBalancesRouteData) => {Based on learnings: TypeScript files should introduce specific interfaces/types for response shapes instead of using
any, though full API response typing can be tracked as a separate enhancement.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/loans/loans-view/working-capital/loan-balances-tab/loan-balances-tab.component.ts` at line 36, Introduce a small interface to replace the use of `any` on the route data and update the observable subscription to use it: add an interface (e.g. `LoanBalancesRouteData`) near the top of the file describing `{ loanDetailsData: { currency: { code: string }; balance: WorkingCapitalBalances } }`, then change the subscription signature from `(data: { loanDetailsData: any })` to `(data: LoanBalancesRouteData)` in the `this.route.parent.data.pipe(...).subscribe(...)` call so `loanDetailsData` is strongly typed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/app/loans/common-resolvers/loans-account-transaction.resolver.ts`:
- Around line 36-43: The resolver's resolve method must reject null route params
before numeric coercion and always return an Observable; update resolve(route:
ActivatedRouteSnapshot) to first check that both loanId and transactionId are
not null (e.g., if (loanId == null || transactionId == null) return
throwError(() => new Error('Missing route params'))), then coerce to numbers and
validate with isNaN before calling
this.loansService.getLoansAccountTransaction(this.loanAccountPath, loanId,
transactionId); ensure every code path returns an Observable (use
throwError/EMPTY) so resolve never falls through returning undefined.
In `@src/app/loans/loans-view/general-tab/general-tab.component.ts`:
- Around line 77-82: The current use of this.loanDetails.transactions.some(...)
never returns true from the callback so it doesn't short-circuit; change it to
assign the boolean result to this.hasChargeBack by returning true when a match
is found (e.g., this.hasChargeBack =
this.loanDetails.transactions.some((transaction: any) => transaction.type.code
=== 'loanTransactionType.chargeback')), removing the manual flag set and the
ineffective return; update code around the some call in general-tab.component
(look for this.hasChargeBack and this.loanDetails.transactions.some)
accordingly.
In
`@src/app/loans/loans-view/general-tab/loan-summary-balance-component/loan-summary-balance-component.component.ts`:
- Around line 50-56: The component currently uses any for the `@Input`() summary
and the MatTableDataSource which defeats TypeScript checks; introduce explicit
interfaces (e.g., LoanSummaryPayload and LoanSummaryRow) that describe the
summary API and each table row (fields like loanId, balance, currency, status,
chargeBackFlag, etc. matching the component's column usage), change the `@Input`()
signature to summary: LoanSummaryPayload | null and change dataSource to
MatTableDataSource<LoanSummaryRow>, update any code that maps/creates rows to
return LoanSummaryRow objects and adjust loanSummaryColumns typing if you can
narrow it (e.g., readonly string[] or a union of column keys) so the table
binding is strictly typed.
In
`@src/app/loans/loans-view/loan-account-actions/make-repayment/make-repayment.component.html`:
- Around line 176-185: The CTA currently uses a hardcoded
*mifosxHasPermission="'REPAYMENT_LOAN'" which is incorrect for actions other
than repayment; update the template to derive the required permission from the
selected action/command (e.g. use a component getter or method that maps the
current command or actionName in make-repayment.component.ts to the correct
permission string) and bind that result to *mifosxHasPermission (keep the
existing [disabled]="!repaymentLoanForm.valid || isSubmitting" logic). Ensure
the mapping function is named clearly (e.g.
getPermissionForAction(command|actionName)) and referenced in the template so
the button only shows for the appropriate permission per action.
- Around line 64-69: Replace the hardcoded label on the mifosx-input-amount
component in make-repayment.component.html: change the inputLabel="'Transaction
Amount'" to use the ngx-translate key (e.g. a namespaced key like
'MAKE_REPAYMENT.TRANSACTION_AMOUNT') and the translate pipe or bound translate
value so the label is translated at runtime; update the translation JSON (e.g.
en.json) with the corresponding key and value to complete the i18n change.
In
`@src/app/loans/loans-view/loan-account-actions/make-repayment/make-repayment.component.ts`:
- Around line 362-378: The error path of waivePenalties currently calls
submitCommandAction(data) while data.transactionAmount has already been reduced
by waived amounts; locate where data is constructed and used in
make-repayment.component.ts and on the waivePenalties(...).subscribe.error
handler either (a) abort the flow (do not call submitCommandAction) or (b)
restore the original transaction amount on data before calling
submitCommandAction; specifically update the waivePenalties error branch in the
block guarded by loanProductService.isLoanProduct && this.isRepayment() to
ensure submitCommandAction receives the unmodified amount (or skip submission) —
adjust references to data, submitCommandAction(), and waivePenalties()
accordingly.
- Around line 373-376: The alert message in make-repayment.component.ts is
hardcoded ("Some penalties could not be waived..."); replace it with an i18n
lookup using TranslateService from `@ngx-translate/core` (e.g., inject
TranslateService in the component and call
this.translate.instant('loan.repayment.penalties_not_waived') or
this.translate.get(...).subscribe(...)) and pass the translated string to
this.alertService.alert({ type: 'Warning', message: translatedText });
add/update the translation key (e.g., loan.repayment.penalties_not_waived) in
the appropriate translation files.
- Around line 105-115: resolveCommandFromActionName currently returns an empty
string on a miss which allows submitCommandAction to post an empty command;
update it to fail-fast by either (A) making the map exhaustive (e.g., add the
missing 'Capitalized Income Adjustment': 'capitalizedIncomeAdjustment' entry to
the map used in resolveCommandFromActionName) or (B) change
resolveCommandFromActionName to return undefined/null or throw when the
actionName is not found and then add a guard in submitCommandAction that checks
the resolved value (from resolveCommandFromActionName) and aborts/throws if
undefined so this.command is never posted when unmapped; reference
resolveCommandFromActionName, submitCommandAction, and isCapitalizedIncome when
implementing the fix.
In `@src/app/loans/loans-view/loans-view.component.html`:
- Line 39: Replace the hardcoded aria-label on the action button in
loans-view.component.html with a translated key using `@ngx-translate/core`; e.g.
change aria-label="Loan account actions" to a bound attribute like
[attr.aria-label]="'LOANS.ACTIONS_ARIA' | translate" on the same button element,
and add the LOANS.ACTIONS_ARIA entry to your locale JSON files (and update
LoansViewComponent only if you need component-specific keys or fallback
handling).
- Line 13: The status tooltip currently binds directly to
loanDetailsData.status.value, which bypasses loanStatusTooltip() and can
desynchronize tooltip text from the status-dot overrides (e.g.,
charge-off/overdue); update the [statusTooltip] binding to call the existing
loanStatusTooltip(...) method (or a computed wrapper that returns the same value
used by the status-dot) so the tooltip text and dot semantics remain
aligned—locate the template attribute [statusTooltip] and replace its expression
with loanStatusTooltip(...) or the component's computed property that
encapsulates the same logic.
In `@src/app/loans/loans-view/transactions-tab/transactions-tab.component.html`:
- Around line 206-207: The actions menu button (<button ...
[matMenuTriggerFor]="transactionMenu" class="action-button">) currently has an
empty aria-label; replace it with a meaningful, non-empty accessible label such
as aria-label="Open transaction actions menu" (or similar concise description)
so screen readers announce the button purpose; update the aria-label attribute
on the button element in transactions-tab.component.html accordingly.
In `@src/app/loans/loans-view/transactions-tab/transactions-tab.component.scss`:
- Around line 22-45: The SCSS uses hardcoded spacing/sizing that breaks the 8px
grid—replace explicit values in .toggle-group (gap: 1.5rem), the parent padding
(0.75rem 0 1rem), .export-button (letter-spacing and border-color), and
.table-wrapper (border-radius: 6px, box-shadow offsets) with the shared design
tokens/SCSS variables from src/main.scss and src/theme/mifosx-theme.scss and
adjust values to the 8px grid (multiples of 8px or the corresponding rem
variable); ensure you swap hardcoded colors/border variables for the theme
variables and use spacing variables (e.g., $space-1, $space-2 or equivalent) or
create/align to the nearest 8px-based token for all other occurrences mentioned
(lines 53-79, 86-94, 147-152, 288-290) so the classes .toggle-group,
.export-button, .table-wrapper and surrounding paddings follow the shared
tokens.
In `@src/app/loans/loans-view/transactions-tab/transactions-tab.component.ts`:
- Around line 326-329: The "Hide Reversed" filter currently only checks
transaction.manuallyReversed while loanTransactionBorderClass treats both
manuallyReversed and reversed as reversed; update the filtering logic that
builds the visible transactions (the function/method where you filter by
manuallyReversed) to also consider transaction.reversed (e.g., treat reversed ||
manuallyReversed as reversed) so the Hide Reversed behavior matches
loanTransactionBorderClass's row state for both API-reversed and
manually-reversed entries.
In
`@src/app/loans/loans-view/transactions/view-transaction/view-transaction.component.ts`:
- Line 91: The nullable property transactionType in ViewTransactionComponent is
dereferenced in the constructor and action methods (e.g.,
this.transactionType.reAge, reAmortize, contractTermination, value, isWriteOff,
and calls to allowUndoTransaction/allowChargebackTransaction) without null
checks; guard against a missing transactionData.type by either (a)
asserting/validating and throwing or logging + returning early if
transactionData.type is absent in the constructor, or (b) narrowing before each
use (if (this.transactionType) { ... }) so callers like
isWriteOff(this.transactionType) and
allowUndoTransaction/allowChargebackTransaction get a non-null
LoanTransactionType; locate references to transactionType, transactionData.type,
isWriteOff, allowUndoTransaction, allowChargebackTransaction and ensure
consistent null-handling or default assignment.
In
`@src/app/loans/loans-view/working-capital/loan-amortization-schedule-tab/loan-amortization-schedule-tab.component.html`:
- Line 29: The template uses the custom pipe usage "{{ 'Payment Date' |
translateKey: 'inputs' }}" which is inconsistent with the rest of the table that
uses the standard "'labels.inputs.X' | translate" pattern; replace the
translateKey usage with the same pattern (e.g., use "'labels.inputs.paymentDate'
| translate" or the project's existing labels.inputs key for Payment Date) so
the column follows the same translate pipe as the other columns and update any
key name to match the translation file if necessary.
In `@src/app/loans/loans.service.ts`:
- Around line 651-652: Change getLoansAccountTransaction's productType parameter
from string to the narrower union 'loans' | 'working-capital-loans' and create
or reuse a shared exported type (e.g., LoanAccountPath) for that union; then
update loanAccountPath return types in LoanProductService (loan-product.service)
and LoanBaseResolver (loan-base.resolver) to return/declare that same
LoanAccountPath type so callers (like the resolver) are type-safe without casts
and all references (getLoansAccountTransaction, loanAccountPath,
LoanProductService, LoanBaseResolver) use the identical union type.
In `@src/app/shared/account-header/account-header.component.scss`:
- Around line 26-27: The status dot in account-header.component.scss currently
uses hardcoded width: 9px and height: 9px; change these to the 8px grid token
instead of explicit pixels (replace 9px with the appropriate design token SCSS
variable or mixin used in the repo, e.g., the 8px sizing token like $space-8 or
$size-8) so the status dot adheres to the 8px grid system; update both width and
height where they appear and ensure you import/use the existing token variable
consistent with other styles.
- Around line 17-18: The stylesheet uses unnecessary ::ng-deep on selectors and
a status dot sized off the 8px grid; remove the ::ng-deep prefix from the
.mat-mdc-card-title and .mat-mdc-card-subtitle selectors so they are scoped to
this component, and change the .status-dot dimensions from 9px to 8px (both
width and height) to comply with the 8px grid.
---
Nitpick comments:
In
`@src/app/loans/loans-view/transactions/export-transactions/export-transactions.component.scss`:
- Around line 9-11: The form selector in export-transactions.component.scss uses
height: 170px which breaks the 8px grid; change the height value in the form
rule (in export-transactions.component.scss) to an 8px-multiple such as 168px
(21×8) or 176px (22×8) to restore grid alignment and visual consistency across
the app.
- Line 15: The rule uses hardcoded padding percentages in
export-transactions.component.scss (padding: 1% 0 2%); replace those literal
values with the project's SCSS spacing variables (from src/main.scss or
src/theme/mifosx-theme.scss) to ensure consistent theming—e.g., map 1%/2% to the
appropriate $spacing-* variables, import the theme file at the top of
export-transactions.component.scss if not already imported, and update the
selector (export-transactions component styles) to use the variables instead of
literal percentages.
In
`@src/app/loans/loans-view/working-capital/loan-balances-tab/loan-balances-tab.component.ts`:
- Line 36: Introduce a small interface to replace the use of `any` on the route
data and update the observable subscription to use it: add an interface (e.g.
`LoanBalancesRouteData`) near the top of the file describing `{ loanDetailsData:
{ currency: { code: string }; balance: WorkingCapitalBalances } }`, then change
the subscription signature from `(data: { loanDetailsData: any })` to `(data:
LoanBalancesRouteData)` in the `this.route.parent.data.pipe(...).subscribe(...)`
call so `loanDetailsData` is strongly typed.
In `@src/app/shared/notifications-tray/notifications-tray.component.scss`:
- Around line 24-35: The :host button.mat-mdc-icon-button block uses hardcoded
colors (rgb(...) and `#fff`); update it to use the theme SCSS variables (e.g.
$white) and alpha via rgba() or existing opacity helper variables from your
theme files: replace rgb(255 255 255 / 80%) with rgba($white, 0.8), rgb(255 255
255 / 15%) with rgba($white, 0.15), and `#fff` with $white in the :host
button.mat-mdc-icon-button and its &:hover rule so the component follows the
project's theme variables defined in src/main.scss and
src/theme/mifosx-theme.scss.
In `@src/app/shared/theme-toggle/theme-toggle.component.scss`:
- Around line 11-17: The SCSS in theme-toggle.component.scss uses hardcoded
colors (e.g., rgb(255 255 255 / 72%), rgb(255 255 255 / 15%), `#fff`); update the
styles in this file to use the project SCSS theme variables instead: import the
colour variables (e.g., add `@use` '../../../assets/styles/colours' as *; at the
top) and replace the hardcoded values in the theme-toggle selector and its
&:hover rules with the corresponding variables from the colours/mifosx-theme
files so the component follows the shared theme.
- Line 9: The selector in theme-toggle.component.scss is currently using the
generic host rule `:host button`; replace it with the MDC-specific selector
`:host button.mat-mdc-icon-button` so styles target the Angular Material v20+
MDC icon button class consistently (update any related nested rules or
specificity to match the new selector).
- Around line 9-20: Extract the duplicated icon-button styles from
theme-toggle.component.scss and notifications-tray.component.scss into a shared
SCSS mixin (e.g., icon-button-hover) in a common mixins.scss, parameterize the
base color opacity (default and override), and replace the duplicated blocks in
both components by importing the mixins file and including the mixin (use the
mixin name icon-button-hover with the appropriate opacity argument for
ThemeToggle and NotificationsTray); update selectors to match existing button
selector (e.g., :host button or :host button.mat-mdc-icon-button) so behavior
and hover states remain identical.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 86c7da73-1dee-43fb-b0d7-86d11fe0743a
📒 Files selected for processing (41)
src/app/loans/common-resolvers/loan-action-button.resolver.tssrc/app/loans/common-resolvers/loans-account-transaction.resolver.tssrc/app/loans/loans-view/general-tab/general-tab.component.htmlsrc/app/loans/loans-view/general-tab/general-tab.component.tssrc/app/loans/loans-view/general-tab/loan-summary-balance-component/loan-summary-balance-component.component.htmlsrc/app/loans/loans-view/general-tab/loan-summary-balance-component/loan-summary-balance-component.component.scsssrc/app/loans/loans-view/general-tab/loan-summary-balance-component/loan-summary-balance-component.component.tssrc/app/loans/loans-view/loan-account-actions/make-repayment/make-repayment.component.htmlsrc/app/loans/loans-view/loan-account-actions/make-repayment/make-repayment.component.tssrc/app/loans/loans-view/loans-view.component.htmlsrc/app/loans/loans-view/loans-view.component.scsssrc/app/loans/loans-view/loans-view.component.tssrc/app/loans/loans-view/transactions-tab/transactions-tab.component.htmlsrc/app/loans/loans-view/transactions-tab/transactions-tab.component.scsssrc/app/loans/loans-view/transactions-tab/transactions-tab.component.tssrc/app/loans/loans-view/transactions/export-transactions/export-transactions.component.htmlsrc/app/loans/loans-view/transactions/export-transactions/export-transactions.component.scsssrc/app/loans/loans-view/transactions/view-transaction/view-transaction.component.htmlsrc/app/loans/loans-view/transactions/view-transaction/view-transaction.component.scsssrc/app/loans/loans-view/transactions/view-transaction/view-transaction.component.tssrc/app/loans/loans-view/working-capital/loan-amortization-schedule-tab/loan-amortization-schedule-tab.component.htmlsrc/app/loans/loans-view/working-capital/loan-balances-tab/loan-balances-tab.component.tssrc/app/loans/loans.service.tssrc/app/shared/account-header/account-header.component.htmlsrc/app/shared/account-header/account-header.component.scsssrc/app/shared/account-header/account-header.component.tssrc/app/shared/notifications-tray/notifications-tray.component.scsssrc/app/shared/theme-toggle/theme-toggle.component.scsssrc/assets/translations/cs-CS.jsonsrc/assets/translations/de-DE.jsonsrc/assets/translations/en-US.jsonsrc/assets/translations/es-CL.jsonsrc/assets/translations/es-MX.jsonsrc/assets/translations/fr-FR.jsonsrc/assets/translations/it-IT.jsonsrc/assets/translations/ko-KO.jsonsrc/assets/translations/lt-LT.jsonsrc/assets/translations/lv-LV.jsonsrc/assets/translations/ne-NE.jsonsrc/assets/translations/pt-PT.jsonsrc/assets/translations/sw-SW.json
Comment on lines
+50
to
+56
| @Input() summary: any | null = null; | ||
| @Input() currency: Currency | null = null; | ||
| @Input() hasChargeBack: boolean = false; | ||
|
|
||
| /** Data source for loans summary table. */ | ||
| dataSource: MatTableDataSource<any>; | ||
| loanSummaryColumns: string[] = []; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Replace any in the new summary-table API with explicit types.
The new component API (summary) and table datasource currently bypass type safety, making field mismatches easy to miss at compile time. Please introduce dedicated interfaces for the summary payload and row model.
Proposed typed shape
+interface LoanSummaryRow {
+ property: string;
+ original: number;
+ paid: number;
+ outstanding: number;
+ adjustment?: number;
+ waived?: number;
+ writtenOff?: number;
+ overdue?: number;
+}
+
+interface LoanSummaryData {
+ totalPrincipal?: number;
+ principalAdjustments?: number;
+ principalPaid?: number;
+ principalWaived?: number;
+ principalWrittenOff?: number;
+ principalOutstanding?: number;
+ principalOverdue?: number;
+ // ...other referenced fields from setLoanSummaryTableData/setWorkingCapitalSummaryTableData
+}
+
- `@Input`() summary: any | null = null;
+ `@Input`() summary: LoanSummaryData | null = null;
...
- dataSource: MatTableDataSource<any>;
+ dataSource: MatTableDataSource<LoanSummaryRow>;As per coding guidelines: src/app/**/*.ts: “Use TypeScript for all application code with strict typing conventions”.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/app/loans/loans-view/general-tab/loan-summary-balance-component/loan-summary-balance-component.component.ts`
around lines 50 - 56, The component currently uses any for the `@Input`() summary
and the MatTableDataSource which defeats TypeScript checks; introduce explicit
interfaces (e.g., LoanSummaryPayload and LoanSummaryRow) that describe the
summary API and each table row (fields like loanId, balance, currency, status,
chargeBackFlag, etc. matching the component's column usage), change the `@Input`()
signature to summary: LoanSummaryPayload | null and change dataSource to
MatTableDataSource<LoanSummaryRow>, update any code that maps/creates rows to
return LoanSummaryRow objects and adjust loanSummaryColumns typing if you can
narrow it (e.g., readonly string[] or a union of column keys) so the table
binding is strictly typed.
Comment on lines
+176
to
+185
| <mat-card-actions class="layout-row align-center gap-5px responsive-column"> | ||
| <button type="button" mat-raised-button (click)="gotoLoanDefaultView()"> | ||
| {{ 'labels.buttons.Cancel' | translate }} | ||
| </button> | ||
| <button | ||
| mat-raised-button | ||
| color="primary" | ||
| [disabled]="!repaymentLoanForm.valid || isSubmitting" | ||
| *mifosxHasPermission="'REPAYMENT_LOAN'" | ||
| > |
There was a problem hiding this comment.
Submit permission needs to follow the selected action.
This component now submits multiple commands (goodwillCredit, capitalizedIncome, buyDownFee, etc. in make-repayment.component.ts), but the button is still gated by REPAYMENT_LOAN. That will block valid non-repayment actions or show the CTA under the wrong permission set. Derive the permission from command or actionName here.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/app/loans/loans-view/loan-account-actions/make-repayment/make-repayment.component.html`
around lines 176 - 185, The CTA currently uses a hardcoded
*mifosxHasPermission="'REPAYMENT_LOAN'" which is incorrect for actions other
than repayment; update the template to derive the required permission from the
selected action/command (e.g. use a component getter or method that maps the
current command or actionName in make-repayment.component.ts to the correct
permission string) and bind that result to *mifosxHasPermission (keep the
existing [disabled]="!repaymentLoanForm.valid || isSubmitting" logic). Ensure
the mapping function is named clearly (e.g.
getPermissionForAction(command|actionName)) and referenced in the template so
the button only shows for the appropriate permission per action.
| <ng-container matColumnDef="paymentDate"> | ||
| <th mat-header-cell class="center mat-header-cell" *matHeaderCellDef> | ||
| {{ 'labels.inputs.Payment Date' | translate }} | ||
| {{ 'Payment Date' | translateKey: 'inputs' }} |
There was a problem hiding this comment.
Inconsistent translation pattern.
This column now uses translateKey: 'inputs' while all other columns in the table (Expected Payment Amount, Discount Factor, NPV Value, etc.) continue to use the standard 'labels.inputs.X' | translate pattern. This inconsistency could cause maintenance issues and should be unified to use one approach throughout the template.
🔧 Proposed fix to restore consistency
- {{ 'Payment Date' | translateKey: 'inputs' }}
+ {{ 'labels.inputs.Payment Date' | translate }}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/app/loans/loans-view/working-capital/loan-amortization-schedule-tab/loan-amortization-schedule-tab.component.html`
at line 29, The template uses the custom pipe usage "{{ 'Payment Date' |
translateKey: 'inputs' }}" which is inconsistent with the rest of the table that
uses the standard "'labels.inputs.X' | translate" pattern; replace the
translateKey usage with the same pattern (e.g., use "'labels.inputs.paymentDate'
| translate" or the project's existing labels.inputs key for Payment Date) so
the column follows the same translate pipe as the other columns and update any
key name to match the translation file if necessary.
alberto-art3ch
force-pushed
the
WEB-657/loans-view-working-capital
branch
from
0d785cc to
42f456f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/app/loans/loans-view/transactions-tab/transactions-tab.component.html (1)
206-213:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winThe
aria-labelis not properly bound for translation.The attribute
aria-label="'labels.inputs.Actions' | translate"is a literal string, not a binding. The translate pipe won't be executed, leaving the button with poor accessibility.♿ Proposed fix
<button mat-icon-button [matMenuTriggerFor]="transactionMenu" - aria-label="'labels.inputs.Actions' | translate" + [attr.aria-label]="'labels.inputs.Actions' | translate" class="action-button" >As per coding guidelines:
src/app/**/*.{html,ts}should "Use proper i18n variables from@ngx-translate/corefor all user-facing strings instead of hardcoded text".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/loans/loans-view/transactions-tab/transactions-tab.component.html` around lines 206 - 213, The aria-label on the actions button (the <button> with mat-icon-button and [matMenuTriggerFor]="transactionMenu") is currently a literal string so the translate pipe won't run; change it to a bound attribute so `@ngx-translate` can execute (use an attribute binding such as [attr.aria-label] with the translate pipe or the translate directive) so the translated label is applied for accessibility.src/app/loans/loans-view/loans-view.component.html (1)
35-46:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winThe
aria-labelis not properly bound for translation.The attribute
aria-label="'labels.text.Loan Account Actions' | translate"is a literal string, not a binding. The translate pipe won't be executed.♿ Proposed fix
<button accountMenu mat-icon-button [matMenuTriggerFor]="accountMenu" - aria-label="'labels.text.Loan Account Actions' | translate" + [attr.aria-label]="'labels.text.Loan Account Actions' | translate" yPosition="below" >As per coding guidelines:
src/app/**/*.{html,ts}should "Use proper i18n variables from@ngx-translate/corefor all user-facing strings instead of hardcoded text".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/loans/loans-view/loans-view.component.html` around lines 35 - 46, The aria-label is currently a literal string so the translate pipe isn't executed; update the button element (the one with accountMenu and mat-icon-button and [matMenuTriggerFor]="accountMenu") to use a bound attribute so the translate pipe runs, e.g. use [attr.aria-label] or [aria-label] with the translate pipe (for example: [attr.aria-label]="'labels.text.Loan Account Actions' | translate") so the label is properly localized.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@src/app/loans/loans-view/loans-view.component.html`:
- Around line 35-46: The aria-label is currently a literal string so the
translate pipe isn't executed; update the button element (the one with
accountMenu and mat-icon-button and [matMenuTriggerFor]="accountMenu") to use a
bound attribute so the translate pipe runs, e.g. use [attr.aria-label] or
[aria-label] with the translate pipe (for example:
[attr.aria-label]="'labels.text.Loan Account Actions' | translate") so the label
is properly localized.
In `@src/app/loans/loans-view/transactions-tab/transactions-tab.component.html`:
- Around line 206-213: The aria-label on the actions button (the <button> with
mat-icon-button and [matMenuTriggerFor]="transactionMenu") is currently a
literal string so the translate pipe won't run; change it to a bound attribute
so `@ngx-translate` can execute (use an attribute binding such as
[attr.aria-label] with the translate pipe or the translate directive) so the
translated label is applied for accessibility.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dd20d4d3-efd1-439e-a98b-674a97bfae99
📒 Files selected for processing (41)
src/app/loans/common-resolvers/loan-action-button.resolver.tssrc/app/loans/common-resolvers/loans-account-transaction.resolver.tssrc/app/loans/loans-view/general-tab/general-tab.component.htmlsrc/app/loans/loans-view/general-tab/general-tab.component.tssrc/app/loans/loans-view/general-tab/loan-summary-balance-component/loan-summary-balance-component.component.htmlsrc/app/loans/loans-view/general-tab/loan-summary-balance-component/loan-summary-balance-component.component.scsssrc/app/loans/loans-view/general-tab/loan-summary-balance-component/loan-summary-balance-component.component.tssrc/app/loans/loans-view/loan-account-actions/make-repayment/make-repayment.component.htmlsrc/app/loans/loans-view/loan-account-actions/make-repayment/make-repayment.component.tssrc/app/loans/loans-view/loans-view.component.htmlsrc/app/loans/loans-view/loans-view.component.scsssrc/app/loans/loans-view/loans-view.component.tssrc/app/loans/loans-view/transactions-tab/transactions-tab.component.htmlsrc/app/loans/loans-view/transactions-tab/transactions-tab.component.scsssrc/app/loans/loans-view/transactions-tab/transactions-tab.component.tssrc/app/loans/loans-view/transactions/export-transactions/export-transactions.component.htmlsrc/app/loans/loans-view/transactions/export-transactions/export-transactions.component.scsssrc/app/loans/loans-view/transactions/view-transaction/view-transaction.component.htmlsrc/app/loans/loans-view/transactions/view-transaction/view-transaction.component.scsssrc/app/loans/loans-view/transactions/view-transaction/view-transaction.component.tssrc/app/loans/loans-view/working-capital/loan-amortization-schedule-tab/loan-amortization-schedule-tab.component.htmlsrc/app/loans/loans-view/working-capital/loan-balances-tab/loan-balances-tab.component.tssrc/app/loans/loans.service.tssrc/app/shared/account-header/account-header.component.htmlsrc/app/shared/account-header/account-header.component.scsssrc/app/shared/account-header/account-header.component.tssrc/app/shared/notifications-tray/notifications-tray.component.scsssrc/app/shared/theme-toggle/theme-toggle.component.scsssrc/assets/translations/cs-CS.jsonsrc/assets/translations/de-DE.jsonsrc/assets/translations/en-US.jsonsrc/assets/translations/es-CL.jsonsrc/assets/translations/es-MX.jsonsrc/assets/translations/fr-FR.jsonsrc/assets/translations/it-IT.jsonsrc/assets/translations/ko-KO.jsonsrc/assets/translations/lt-LT.jsonsrc/assets/translations/lv-LV.jsonsrc/assets/translations/ne-NE.jsonsrc/assets/translations/pt-PT.jsonsrc/assets/translations/sw-SW.json
✅ Files skipped from review due to trivial changes (9)
- src/assets/translations/ne-NE.json
- src/assets/translations/de-DE.json
- src/assets/translations/pt-PT.json
- src/assets/translations/lt-LT.json
- src/assets/translations/fr-FR.json
- src/assets/translations/en-US.json
- src/assets/translations/cs-CS.json
- src/app/loans/loans-view/general-tab/loan-summary-balance-component/loan-summary-balance-component.component.scss
- src/app/loans/loans-view/transactions/export-transactions/export-transactions.component.scss
🚧 Files skipped from review as they are similar to previous changes (26)
- src/app/shared/account-header/account-header.component.html
- src/app/shared/notifications-tray/notifications-tray.component.scss
- src/app/shared/theme-toggle/theme-toggle.component.scss
- src/assets/translations/sw-SW.json
- src/app/loans/loans-view/transactions/export-transactions/export-transactions.component.html
- src/app/shared/account-header/account-header.component.ts
- src/assets/translations/lv-LV.json
- src/app/loans/common-resolvers/loans-account-transaction.resolver.ts
- src/assets/translations/es-MX.json
- src/app/loans/loans-view/transactions/view-transaction/view-transaction.component.scss
- src/app/loans/loans-view/general-tab/general-tab.component.html
- src/assets/translations/es-CL.json
- src/assets/translations/ko-KO.json
- src/app/loans/loans-view/general-tab/loan-summary-balance-component/loan-summary-balance-component.component.html
- src/app/loans/loans-view/loans-view.component.scss
- src/app/loans/loans-view/loan-account-actions/make-repayment/make-repayment.component.html
- src/assets/translations/it-IT.json
- src/app/loans/loans-view/general-tab/loan-summary-balance-component/loan-summary-balance-component.component.ts
- src/app/loans/loans-view/transactions-tab/transactions-tab.component.scss
- src/app/loans/loans-view/loans-view.component.ts
- src/app/loans/loans-view/working-capital/loan-balances-tab/loan-balances-tab.component.ts
- src/app/loans/loans-view/general-tab/general-tab.component.ts
- src/app/loans/loans-view/transactions/view-transaction/view-transaction.component.html
- src/app/loans/loans-view/loan-account-actions/make-repayment/make-repayment.component.ts
- src/app/loans/loans-view/transactions-tab/transactions-tab.component.ts
- src/app/loans/loans-view/transactions/view-transaction/view-transaction.component.ts
alberto-art3ch
force-pushed
the
WEB-657/loans-view-working-capital
branch
from
42f456f to
41cd56d
Compare
|
Actionable comments posted: 0 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Loan view enhanced and Repayment command for Working Capital
Related issues and discussion
WEB-657
Screenshots, if any
Screen.Recording.2026-05-27.at.11.25.06.PM.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
Improvements
Internationalization