fix: Loan repayment calendar showing wrong dates and status (MIFOSAC-705) - #2618
fix: Loan repayment calendar showing wrong dates and status (MIFOSAC-705)#2618therajanmaurya merged 2 commits into
Conversation
MIFOSAC-705: Fixed the Schedule (Calendar) step in new loan application showing wrong dates and installment statuses. Root cause: repaymentScheduler() was fetching an existing loan by client ID instead of calculating a preview based on the form inputs. Changes: - Added calculateLoanSchedule API endpoint (POST /loans?command=calculateLoanSchedule) - Added CalculateLoanScheduleUseCase for calculating schedule preview - Updated NewLoanAccountViewModel to use form data for schedule calculation - Schedule now correctly shows dates entered in Terms step - All installments show as pending (not paid/due from old loan data) Files changed: - core/network: Added calculateLoanSchedule endpoint to LoanService - core/data: Added method to LoanAccountRepository - core/domain: Created CalculateLoanScheduleUseCase - feature/loan: Updated NewLoanAccountViewModel Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds a loan schedule preview flow: new API endpoint and data manager method, repository and use-case plumbing, ViewModel wiring to build a LoansPayload and request a RepaymentSchedule, plus related UI/navigation and model tweaks. Changes
Sequence DiagramsequenceDiagram
participant UI as UI Layer
participant VM as NewLoanAccountViewModel
participant UC as CalculateLoanScheduleUseCase
participant Repo as LoanAccountRepository
participant DM as DataManagerLoan
participant API as LoanService
UI->>VM: Request schedule preview
VM->>VM: buildLoansPayloadForSchedulePreview()
VM->>UC: invoke(loansPayload)
UC->>Repo: calculateLoanSchedule(loansPayload)
Repo->>DM: calculateLoanSchedule(loansPayload)
DM->>API: POST /loans?command=calculateLoanSchedule
API-->>DM: HttpResponse
DM-->>Repo: RepaymentSchedule (parsed)
Repo-->>UC: Flow<DataState<RepaymentSchedule>>
UC-->>VM: Flow<DataState<RepaymentSchedule>>
VM-->>UI: Update repaymentSchedule / summary
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountViewModel.kt (1)
255-261:⚠️ Potential issue | 🟡 MinorStale schedule preview when form terms are changed and the step is revisited.
The
if (state.repaymentSchedules.isEmpty())guard means the schedule is fetched exactly once per ViewModel instance. If the user views the schedule, navigates back, changes any term (e.g.,numberOfRepayments,principalAmount,expectedDisbursementDate), and navigates forward again, they will see the preview built from the original inputs — directly contradicting the intent of this fix.Consider clearing
repaymentScheduleswhenever key terms change (e.g., inhandleNoOfRepaymentsChange,handlePrincipalAmountChange,handleExpectedDisbursementDateChange), so the next visit to the schedule step triggers a fresh calculation:private fun handleNoOfRepaymentsChange(action: NewLoanAccountAction.OnNoOfRepaymentsChange) { mutableStateFlow.update { - it.copy(noOfRepayments = action.number) + it.copy(noOfRepayments = action.number, repaymentSchedules = emptyMap()) } }Apply the same pattern to
handlePrincipalAmountChange,handleNominalInterestRateChange,handleRepaidEveryChange,handleExpectedDisbursementDateChange, andhandleTermFrequencyIndexChange.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountViewModel.kt` around lines 255 - 261, The repayment preview is stale because NewLoanAccountAction.RepaymentScheduler only calls repaymentScheduler() when state.repaymentSchedules.isEmpty(); fix by invalidating/clearing repaymentSchedules in each term-change handler so revisiting the schedule step forces recalculation: update handleNoOfRepaymentsChange, handlePrincipalAmountChange, handleNominalInterestRateChange, handleRepaidEveryChange, handleExpectedDisbursementDateChange, and handleTermFrequencyIndexChange to reset state.repaymentSchedules (e.g., to emptyList()) whenever their value changes before returning, so the existing isEmpty() guard will trigger a fresh repaymentScheduler() call on next visit.
🧹 Nitpick comments (1)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountViewModel.kt (1)
938-999: Duplicate payload-building logic betweenbuildLoansPayloadForSchedulePreview()andsubmitLoanApplication().The two blocks construct a nearly identical
LoansPayload. The main difference is that the preview helper correctly usestoDoubleOrNull()andgetOrNull()for safety, whilesubmitLoanApplication()usestoDouble()and direct index access (potentialNumberFormatException/IndexOutOfBoundsExceptionif input is invalid). A shared helper parameterised for strict vs. lenient parsing would eliminate the duplication and propagate the null-safety improvements to the submit path as well.♻️ Suggested extraction
private fun buildLoansPayload(strict: Boolean = false): LoansPayload { return LoansPayload( principal = if (strict) state.principalAmount.toDouble() else state.principalAmount.toDoubleOrNull(), interestRatePerPeriod = if (strict) state.nominalInterestRate.toDouble() else state.nominalInterestRate.toDoubleOrNull(), // ... all shared fields using getOrNull() ... ) } // submitLoanApplication → buildLoansPayload(strict = true) // schedule preview → buildLoansPayload(strict = false)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountViewModel.kt` around lines 938 - 999, There is duplicated LoansPayload construction between buildLoansPayloadForSchedulePreview() and submitLoanApplication() with inconsistent safety (preview uses toDoubleOrNull/getOrNull; submit uses toDouble/direct index access). Extract a single helper (e.g., private fun buildLoansPayload(strict: Boolean = false)) that builds the shared LoansPayload using getOrNull() and toDoubleOrNull() when strict=false and toDouble()/direct access when strict=true, then call buildLoansPayload(strict = false) from buildLoansPayloadForSchedulePreview() and buildLoansPayload(strict = true) from submitLoanApplication(); ensure all fields shown (principal, interestRatePerPeriod, loanOfficerId, repaymentFrequency*, loanPurposeId, fundId, linkAccountId, transactionProcessingStrategyCode, etc.) follow this pattern.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@core/network/src/commonMain/kotlin/com/mifos/core/network/services/LoanService.kt`:
- Around line 85-86: The POST handler calculateLoanSchedule currently
deserializes the API response into LoanWithAssociationsEntity which expects
repaymentSchedule.periods nested under repaymentSchedule, but the Fineract
endpoint returns currency and periods at the top level; update the Retrofit
service to use a dedicated response model (e.g., LoanScheduleResponse) matching
the API shape, or implement a custom deserializer that maps top-level currency
and periods into LoanWithAssociationsEntity.repaymentSchedule.periods before
returning the Flow, ensuring SchedulePage’s access to
state.loanWithAssociationsEntity.repaymentSchedule.periods.orEmpty() receives
the actual period data.
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountViewModel.kt`:
- Around line 907-918: The map construction inside the DataState.Success branch
(schedulerDetails in NewLoanAccountViewModel) dereferences
dataState.data.currency directly; change those accesses to null-safe calls and
provide defaults (e.g., use dataState.data.currency?.code ?: "N/A" and
dataState.data.currency?.decimalPlaces ?: 0) so CurrencyFormatter.format and
other string interpolations never receive a null and cannot throw an NPE.
---
Outside diff comments:
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountViewModel.kt`:
- Around line 255-261: The repayment preview is stale because
NewLoanAccountAction.RepaymentScheduler only calls repaymentScheduler() when
state.repaymentSchedules.isEmpty(); fix by invalidating/clearing
repaymentSchedules in each term-change handler so revisiting the schedule step
forces recalculation: update handleNoOfRepaymentsChange,
handlePrincipalAmountChange, handleNominalInterestRateChange,
handleRepaidEveryChange, handleExpectedDisbursementDateChange, and
handleTermFrequencyIndexChange to reset state.repaymentSchedules (e.g., to
emptyList()) whenever their value changes before returning, so the existing
isEmpty() guard will trigger a fresh repaymentScheduler() call on next visit.
---
Nitpick comments:
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountViewModel.kt`:
- Around line 938-999: There is duplicated LoansPayload construction between
buildLoansPayloadForSchedulePreview() and submitLoanApplication() with
inconsistent safety (preview uses toDoubleOrNull/getOrNull; submit uses
toDouble/direct index access). Extract a single helper (e.g., private fun
buildLoansPayload(strict: Boolean = false)) that builds the shared LoansPayload
using getOrNull() and toDoubleOrNull() when strict=false and toDouble()/direct
access when strict=true, then call buildLoansPayload(strict = false) from
buildLoansPayloadForSchedulePreview() and buildLoansPayload(strict = true) from
submitLoanApplication(); ensure all fields shown (principal,
interestRatePerPeriod, loanOfficerId, repaymentFrequency*, loanPurposeId,
fundId, linkAccountId, transactionProcessingStrategyCode, etc.) follow this
pattern.
| @POST(APIEndPoint.CREATE_LOANS_ACCOUNTS + "?command=calculateLoanSchedule") | ||
| fun calculateLoanSchedule(@Body loansPayload: LoansPayload?): Flow<LoanWithAssociationsEntity> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the LoanWithAssociationsEntity definition
echo "=== LoanWithAssociationsEntity definition ==="
rg -nP "class LoanWithAssociationsEntity" --type=kotlin -A 80
echo -e "\n=== Check for repaymentSchedule field ==="
rg -nP "repaymentSchedule" --type=kotlin -B 3 -A 3 | head -100
echo -e "\n=== Check syncLoanById method ==="
rg -nP "fun syncLoanById" --type=kotlin -A 10Repository: openMF/android-client
Length of output: 33041
🏁 Script executed:
#!/bin/bash
# Check the LoanScheduleData or similar response structure
echo "=== Search for LoanScheduleData or schedule-related entities ==="
rg -nP "class Loan.*Schedule|data class.*Schedule" --type=kotlin -A 20
echo -e "\n=== Check the LoansPayload structure ==="
rg -nP "class LoansPayload|data class LoansPayload" --type=kotlin -A 30Repository: openMF/android-client
Length of output: 21504
🏁 Script executed:
#!/bin/bash
# Check the DataManagerLoan implementation
echo "=== Check DataManagerLoan.calculateLoanSchedule call ==="
rg -nP "calculateLoanSchedule" --type=kotlin -B 5 -A 10Repository: openMF/android-client
Length of output: 11692
The response type mismatch requires investigation or fixing.
The Fineract POST /loans?command=calculateLoanSchedule endpoint returns a flat JSON structure with currency and periods at the top level, but the code deserializes it into LoanWithAssociationsEntity, which expects these fields nested under repaymentSchedule. After deserialization, repaymentSchedule.periods will be null or empty, even though the API returned the period data. This breaks the repayment schedule preview shown in the loan account creation flow (SchedulePage accesses state.loanWithAssociationsEntity.repaymentSchedule.periods.orEmpty()).
Consider creating a dedicated response type for the calculateLoanSchedule endpoint (e.g., LoanScheduleResponse) that matches the actual API response structure, or verify that custom JSON deserialization logic exists to handle the structural mismatch.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@core/network/src/commonMain/kotlin/com/mifos/core/network/services/LoanService.kt`
around lines 85 - 86, The POST handler calculateLoanSchedule currently
deserializes the API response into LoanWithAssociationsEntity which expects
repaymentSchedule.periods nested under repaymentSchedule, but the Fineract
endpoint returns currency and periods at the top level; update the Retrofit
service to use a dedicated response model (e.g., LoanScheduleResponse) matching
the API shape, or implement a custom deserializer that maps top-level currency
and periods into LoanWithAssociationsEntity.repaymentSchedule.periods before
returning the Flow, ensuring SchedulePage’s access to
state.loanWithAssociationsEntity.repaymentSchedule.periods.orEmpty() receives
the actual period data.
|
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/pages/SchedulePage.kt (1)
90-108:⚠️ Potential issue | 🔴 Critical
RepaymentPeriodCardwill crash on Android/Desktop withIllegalArgumentExceptionwhencurrencyCodeis null.The component passes
currencyCode ?: "N/A"toCurrencyFormatter.format(), but"N/A"is not a valid ISO 4217 currency code. On Android and Desktop platforms,Currency.getInstance("N/A")throwsIllegalArgumentException. The Native implementation correctly usescurrencyCode ?: "$"as its fallback. Align all platform implementations to use a valid currency code as the default, or add null-safety at the call site.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/pages/SchedulePage.kt` around lines 90 - 108, RepaymentPeriodCard crashes when currencyCode is null because CurrencyFormatter.format receives "N/A" (invalid ISO code); update the call site in RepaymentScheduleList (and any callers of RepaymentPeriodCard) to pass a valid fallback like "$" instead of "N/A" or add null-safety before calling CurrencyFormatter.format so it never invokes Currency.getInstance with an invalid code; specifically change usages that currently propagate currencyCode ?: "N/A" to use currencyCode ?: "$" (or guard and format with a currency-symbol-only path) so CurrencyFormatter.format always receives a valid currency identifier.feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientApplyNewApplications/ClientApplyNewApplicationRoute.kt (1)
17-22:⚠️ Potential issue | 🟡 MinorAdd a default value to
accountNofor process-death back-stack safety.When navigating, the route is saved to persist system-initiated process death and the saving mechanism is a binder transaction. With
accountNo: Stringdeclared without a default, any Navigation back-stack state that was serialized under the previous schema (e.g., from an earlier install still on the device) will throw akotlinx.serialization.MissingFieldExceptionwhen the OS tries to restore it viatoRoute<ClientApplyNewApplicationRoute>(). Adding= ""costs nothing and makes the schema forward-compatible.🛡️ Proposed fix
data class ClientApplyNewApplicationRoute( val clientId: Int, val status: String, - val accountNo: String, + val accountNo: String = "", )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientApplyNewApplications/ClientApplyNewApplicationRoute.kt` around lines 17 - 22, The ClientApplyNewApplicationRoute data class must provide a default for accountNo to avoid kotlinx.serialization.MissingFieldException during process-death back-stack restore; update the class definition (ClientApplyNewApplicationRoute) so the accountNo property has a default value (e.g., accountNo = "") while keeping clientId and status unchanged so older serialized routes can be deserialized safely.
🧹 Nitpick comments (3)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/pages/SchedulePage.kt (1)
57-57: Nit: prefer idiomaticisNotEmpty()over!...isEmpty().♻️ Suggested change
- if (!state.repaymentSchedulesSummary.isEmpty()) { + if (state.repaymentSchedulesSummary.isNotEmpty()) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/pages/SchedulePage.kt` at line 57, Replace the negated isEmpty() check with the idiomatic isNotEmpty() on the repaymentSchedulesSummary property: find the conditional using state.repaymentSchedulesSummary (in SchedulePage.kt) and change the expression from "!state.repaymentSchedulesSummary.isEmpty()" to "state.repaymentSchedulesSummary.isNotEmpty()" to improve readability and follow Kotlin conventions.core/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerLoan.kt (1)
305-314:Jsoninstance is recreated on every invocation — allocate once and reuse.A new
Json { ignoreUnknownKeys = true }is instantiated on each call. This is unnecessary overhead. Consider extracting it to acompanion objector a top-levelval, especially sinceextractErrorMessageinErrorHandling.ktalready creates its own. A shared instance would benefit both call sites.♻️ Suggested improvement
+private val lenientJson = Json { ignoreUnknownKeys = true } + fun calculateLoanSchedule(loansPayload: LoansPayload): Flow<RepaymentSchedule> { return mBaseApiManager.loanService.calculateLoanSchedule(loansPayload).map { response -> if (!response.status.isSuccess()) { val errorMessage = extractErrorMessage(response) throw IllegalStateException(errorMessage) } - Json { ignoreUnknownKeys = true }.decodeFromString<RepaymentSchedule>(response.bodyAsText()) + lenientJson.decodeFromString<RepaymentSchedule>(response.bodyAsText()) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerLoan.kt` around lines 305 - 314, The calculateLoanSchedule function currently creates a new Json { ignoreUnknownKeys = true } on every call; extract a single shared Json instance (e.g., a private val json = Json { ignoreUnknownKeys = true }) and reuse it instead of instantiating inside calculateLoanSchedule, update the decodeFromString call to use that shared json to parse RepaymentSchedule, and consider placing the shared val in the same class companion object or a top-level file so ErrorHandling.extractErrorMessage can also reuse it.feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountViewModel.kt (1)
957-1018: Payload construction is largely duplicated withsubmitLoanApplication().
buildLoansPayloadForSchedulePreview()constructs aLoansPayloadvery similarly to lines 275-302 insubmitLoanApplication(), but with safer accessors (getOrNull,toDoubleOrNull). This creates a maintenance risk — if a field is added or changed, both sites must be updated in sync.Consider extracting a shared
buildLoansPayload()method that both functions call. The preview version could use the same builder (the safer one), andsubmitLoanApplicationcould add its own validation on top.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountViewModel.kt` around lines 957 - 1018, Extract a single shared builder function (e.g., buildLoansPayload or buildLoansPayloadSafe) that constructs and returns a LoansPayload using the safe accessors currently used in buildLoansPayloadForSchedulePreview (getOrNull, toDoubleOrNull, nullable indexing), then have both buildLoansPayloadForSchedulePreview() and submitLoanApplication() call that shared builder; keep buildLoansPayloadForSchedulePreview() as a thin wrapper returning the shared result, and in submitLoanApplication() call the shared builder and then perform any additional validation/strict conversions before submission so you avoid duplicating field mapping logic between buildLoansPayloadForSchedulePreview(), submitLoanApplication(), and the LoansPayload construction.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@core/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/Currency.kt`:
- Line 26: The field decimalPlaces was changed to Int? which is correct
semantically but kotlinx.serialization will throw JsonDecodingException if the
API emits a float literal like 2.0; to fix, either (A) confirm and enforce the
API contract to emit integer literals for decimalPlaces, or (B) make
deserialization tolerant by adding a custom serializer (or
JsonTransformingSerializer) for decimalPlaces that accepts both integer and
float JSON numbers and converts them to Int, and apply it where decimalPlaces is
declared in com.mifos.core.model.objects.account.loan.Currency (and update the
other Currency classes template/loan/Currency.kt and template/client/Currency.kt
to use the same Int? type or the same serializer) so all domain models are
consistent and runtime decoding won't crash.
In
`@core/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerLoan.kt`:
- Around line 298-303: The KDoc for the method in DataManagerLoan
(DataManagerLoan.kt) incorrectly documents the return type as
LoanWithAssociationsEntity while the function actually returns a
RepaymentSchedule; update the KDoc `@return` to describe and name
RepaymentSchedule (and adjust the brief description to match the actual returned
object) or, if the implementation was intended to return
LoanWithAssociationsEntity, change the function return type and implementation
accordingly—look for the function calculateLoanRepaymentSchedule / similar in
DataManagerLoan and make the KDoc and signature/implementation consistent.
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountViewModel.kt`:
- Around line 926-936: The schedulerDetails map is using stale
state.repaymentSchedule (which is updated later) causing totalPrincipalPaid to
be wrong; update the map to read totalPrincipalPaid from the fresh response
dataState.data.repaymentSchedule (e.g., use
dataState.data.repaymentSchedule.totalPrincipalPaid) and keep the existing
null-safety for currency code and decimalPlaces when calling
CurrencyFormatter.format; ensure you reference dataState.data rather than state
for repayment values inside the schedulerDetails construction in
NewLoanAccountViewModel.kt.
- Around line 254-266: The current guard in the
NewLoanAccountAction.RepaymentScheduler branch only recalculates the schedule
when state.noOfRepayments changes (checking noOfRepaymentsPreviousState !=
noOfRepayments), which can leave the schedule stale when other inputs change;
either (A) always call repaymentScheduler() when handling
NewLoanAccountAction.RepaymentScheduler / after moveToNextStep() so the schedule
is recalculated whenever the user navigates to the schedule step, or (B) broaden
the staleness check by comparing a snapshot of all schedule-affecting fields
(e.g., principal, interestRate, disbursementDate,
repaymentFrequency/periodicity, loanTerm, fees) stored in state (e.g.,
repaymentSchedulePreviousSnapshot) against the current values and run
repaymentScheduler() when any differ, then update that snapshot via
mutableStateFlow.update; use the existing repaymentScheduler(),
moveToNextStep(), mutableStateFlow.update, and state.* fields to locate and
implement the change.
- Line 1025: The current NewLoanAccountViewModel state property accountNo
defaults to an empty string and can be passed through from
ClientProfileDetailsScreen (via navigateToClientApplyNewApplicationScreen →
ClientApplyNewApplicationsState → navigateToNewLoanAccountRoute), which means
the schedule summary may show no account number; decide whether empty is
acceptable or enforce a fallback/validation: either change the flow to pass a
non-empty fallback (e.g., "N/A" or generate a placeholder) at the caller
(ClientProfileDetailsScreen) before calling
navigateToClientApplyNewApplicationScreen, or add validation in
NewLoanAccountViewModel (or the code that renders the schedule summary) to
detect accountNo == "" and substitute a clear fallback or show a
validation/error state; update the relevant places (ClientProfileDetailsScreen,
navigateToClientApplyNewApplicationScreen, ClientApplyNewApplicationsState,
navigateToNewLoanAccountRoute, and NewLoanAccountViewModel) so the chosen
behavior is consistent.
---
Outside diff comments:
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientApplyNewApplications/ClientApplyNewApplicationRoute.kt`:
- Around line 17-22: The ClientApplyNewApplicationRoute data class must provide
a default for accountNo to avoid kotlinx.serialization.MissingFieldException
during process-death back-stack restore; update the class definition
(ClientApplyNewApplicationRoute) so the accountNo property has a default value
(e.g., accountNo = "") while keeping clientId and status unchanged so older
serialized routes can be deserialized safely.
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/pages/SchedulePage.kt`:
- Around line 90-108: RepaymentPeriodCard crashes when currencyCode is null
because CurrencyFormatter.format receives "N/A" (invalid ISO code); update the
call site in RepaymentScheduleList (and any callers of RepaymentPeriodCard) to
pass a valid fallback like "$" instead of "N/A" or add null-safety before
calling CurrencyFormatter.format so it never invokes Currency.getInstance with
an invalid code; specifically change usages that currently propagate
currencyCode ?: "N/A" to use currencyCode ?: "$" (or guard and format with a
currency-symbol-only path) so CurrencyFormatter.format always receives a valid
currency identifier.
---
Nitpick comments:
In
`@core/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerLoan.kt`:
- Around line 305-314: The calculateLoanSchedule function currently creates a
new Json { ignoreUnknownKeys = true } on every call; extract a single shared
Json instance (e.g., a private val json = Json { ignoreUnknownKeys = true }) and
reuse it instead of instantiating inside calculateLoanSchedule, update the
decodeFromString call to use that shared json to parse RepaymentSchedule, and
consider placing the shared val in the same class companion object or a
top-level file so ErrorHandling.extractErrorMessage can also reuse it.
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountViewModel.kt`:
- Around line 957-1018: Extract a single shared builder function (e.g.,
buildLoansPayload or buildLoansPayloadSafe) that constructs and returns a
LoansPayload using the safe accessors currently used in
buildLoansPayloadForSchedulePreview (getOrNull, toDoubleOrNull, nullable
indexing), then have both buildLoansPayloadForSchedulePreview() and
submitLoanApplication() call that shared builder; keep
buildLoansPayloadForSchedulePreview() as a thin wrapper returning the shared
result, and in submitLoanApplication() call the shared builder and then perform
any additional validation/strict conversions before submission so you avoid
duplicating field mapping logic between buildLoansPayloadForSchedulePreview(),
submitLoanApplication(), and the LoansPayload construction.
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/pages/SchedulePage.kt`:
- Line 57: Replace the negated isEmpty() check with the idiomatic isNotEmpty()
on the repaymentSchedulesSummary property: find the conditional using
state.repaymentSchedulesSummary (in SchedulePage.kt) and change the expression
from "!state.repaymentSchedulesSummary.isEmpty()" to
"state.repaymentSchedulesSummary.isNotEmpty()" to improve readability and follow
Kotlin conventions.
| val name: String? = null, | ||
|
|
||
| val decimalPlaces: Double? = null, | ||
| val decimalPlaces: Int? = null, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the Currency class definition and confirm the change
fd -e kt -x grep -l "class Currency" {} \;Repository: openMF/android-client
Length of output: 705
🏁 Script executed:
# Search for all usages of decimalPlaces across the codebase
rg -n "decimalPlaces" --type kotlin -B 2 -A 2Repository: openMF/android-client
Length of output: 35017
🏁 Script executed:
# Look specifically for CurrencyFormatter to understand how it uses decimal places
fd -e kt "CurrencyFormatter" -x cat -n {}Repository: openMF/android-client
Length of output: 4026
🏁 Script executed:
# Check if there are any .toInt() conversions on decimalPlaces that would become redundant
rg -n "decimalPlaces.*\.toInt\(\)" --type kotlin
# Verify the actual type signature of maximumFractionDigits in CurrencyFormatter to ensure compatibility
rg -n "maximumFractionDigits.*Int" core/common/src/commonMain/kotlin/com/mifos/core/common/utils/ -A 1 -B 1Repository: openMF/android-client
Length of output: 656
🏁 Script executed:
# Check for any Double-typed parameters that receive decimalPlaces
rg -n "fun.*Double.*decimalPlaces|maximumFractionDigits.*Double" --type kotlinRepository: openMF/android-client
Length of output: 252
🏁 Script executed:
# Verify which Currency class is actually being modified in this PR
# and check if there are inconsistencies in how different Currency classes handle decimalPlaces
fd -e kt "Currency.kt" -x grep -l "decimalPlaces.*Int\|decimalPlaces.*Double" {}Repository: openMF/android-client
Length of output: 808
🏁 Script executed:
# Also check the specific file from the review to confirm it's account/loan/Currency.kt
cat -n core/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/Currency.kt | grep -A 2 -B 2 "decimalPlaces"Repository: openMF/android-client
Length of output: 203
Verify API contract ensures integer format for decimalPlaces field.
Changing decimalPlaces from Double? to Int? is semantically correct — decimal-place counts must be integers. This is the right type. However, one critical verification remains:
With kotlinx.serialization, deserializing a JSON float literal ("decimalPlaces": 2.0) into Int? throws JsonDecodingException at runtime. If the Mifos API ever emits float-formatted values for this field, the app will crash during response parsing. Confirm the API contract always returns an integer literal (not 2.0).
Downstream callers all expect Int? for maximumFractionDigits and pass decimalPlaces directly without transformation—no compilation issues. However, note that other Currency classes in the codebase (template/loan/Currency.kt, template/client/Currency.kt) still use Double? for the same field, creating inconsistency across the domain model.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@core/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/Currency.kt`
at line 26, The field decimalPlaces was changed to Int? which is correct
semantically but kotlinx.serialization will throw JsonDecodingException if the
API emits a float literal like 2.0; to fix, either (A) confirm and enforce the
API contract to emit integer literals for decimalPlaces, or (B) make
deserialization tolerant by adding a custom serializer (or
JsonTransformingSerializer) for decimalPlaces that accepts both integer and
float JSON numbers and converts them to Int, and apply it where decimalPlaces is
declared in com.mifos.core.model.objects.account.loan.Currency (and update the
other Currency classes template/loan/Currency.kt and template/client/Currency.kt
to use the same Int? type or the same serializer) so all domain models are
consistent and runtime decoding won't crash.
| /** | ||
| * Calculate loan repayment schedule without creating the loan. | ||
| * Used to preview the schedule before submitting the loan application. | ||
| * | ||
| * @param loansPayload The loan parameters to calculate the schedule for | ||
| * @return LoanWithAssociationsEntity containing the calculated repayment schedule |
There was a problem hiding this comment.
KDoc @return is stale — says LoanWithAssociationsEntity but returns RepaymentSchedule.
- * `@return` LoanWithAssociationsEntity containing the calculated repayment schedule
+ * `@return` RepaymentSchedule containing the calculated repayment schedule📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * Calculate loan repayment schedule without creating the loan. | |
| * Used to preview the schedule before submitting the loan application. | |
| * | |
| * @param loansPayload The loan parameters to calculate the schedule for | |
| * @return LoanWithAssociationsEntity containing the calculated repayment schedule | |
| /** | |
| * Calculate loan repayment schedule without creating the loan. | |
| * Used to preview the schedule before submitting the loan application. | |
| * | |
| * `@param` loansPayload The loan parameters to calculate the schedule for | |
| * `@return` RepaymentSchedule containing the calculated repayment schedule |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@core/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerLoan.kt`
around lines 298 - 303, The KDoc for the method in DataManagerLoan
(DataManagerLoan.kt) incorrectly documents the return type as
LoanWithAssociationsEntity while the function actually returns a
RepaymentSchedule; update the KDoc `@return` to describe and name
RepaymentSchedule (and adjust the brief description to match the actual returned
object) or, if the implementation was intended to return
LoanWithAssociationsEntity, change the function return type and implementation
accordingly—look for the function calculateLoanRepaymentSchedule / similar in
DataManagerLoan and make the KDoc and signature/implementation consistent.
| is NewLoanAccountAction.RepaymentScheduler -> { | ||
| moveToNextStep() | ||
| if (state.repaymentSchedules.isEmpty()) { | ||
| if (state.noOfRepaymentsPreviousState != state.noOfRepayments) { | ||
| viewModelScope.launch { | ||
| repaymentScheduler() | ||
| } | ||
|
|
||
| mutableStateFlow.update { | ||
| it.copy( | ||
| noOfRepaymentsPreviousState = state.noOfRepayments, | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
Schedule recalculation is only triggered when noOfRepayments changes.
The guard at line 256 compares noOfRepaymentsPreviousState != noOfRepayments, but the schedule depends on many other inputs (principal, interest rate, disbursement date, repayment frequency, etc.). If the user changes any of those fields after viewing the schedule once without also changing the number of repayments, they'll see a stale schedule.
Consider either broadening the staleness check to cover all schedule-affecting fields, or always recalculating when the user navigates to the schedule step.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountViewModel.kt`
around lines 254 - 266, The current guard in the
NewLoanAccountAction.RepaymentScheduler branch only recalculates the schedule
when state.noOfRepayments changes (checking noOfRepaymentsPreviousState !=
noOfRepayments), which can leave the schedule stale when other inputs change;
either (A) always call repaymentScheduler() when handling
NewLoanAccountAction.RepaymentScheduler / after moveToNextStep() so the schedule
is recalculated whenever the user navigates to the schedule step, or (B) broaden
the staleness check by comparing a snapshot of all schedule-affecting fields
(e.g., principal, interestRate, disbursementDate,
repaymentFrequency/periodicity, loanTerm, fees) stored in state (e.g.,
repaymentSchedulePreviousSnapshot) against the current values and run
repaymentScheduler() when any differ, then update that snapshot via
mutableStateFlow.update; use the existing repaymentScheduler(),
moveToNextStep(), mutableStateFlow.update, and state.* fields to locate and
implement the change.
| val schedulerDetails = mapOf( | ||
| Res.string.account_number to dataState.data.accountNo, | ||
| Res.string.disbursement_date to ( | ||
| dataState.data.timeline.actualDisbursementDate?.filterNotNull() | ||
| ?.let { date -> | ||
| DateHelper.getDateAsString(date) | ||
| } ?: "N/A" | ||
| ), | ||
| Res.string.account_number to (state.accountNo), | ||
| Res.string.disbursement_date to state.expectedDisbursementDate.ifEmpty { "N/A" }, | ||
| Res.string.principle_paid_off to CurrencyFormatter.format( | ||
| balance = dataState.data.summary.principalPaid ?: 0.0, | ||
| currencyCode = dataState.data.currency.code ?: "N/A", | ||
| maximumFractionDigits = dataState.data.currency.decimalPlaces ?: 0, | ||
| balance = state.repaymentSchedule.totalPrincipalPaid, | ||
| currencyCode = dataState.data.currency?.code ?: "N/A", | ||
| maximumFractionDigits = dataState.data.currency?.decimalPlaces ?: 0, | ||
| ), | ||
| Res.string.installment_paid to ( | ||
| dataState.data.repaymentSchedule.periods?.count { it.complete == true } | ||
| ?.toString() ?: "N/A" | ||
| ), | ||
| Res.string.installment_paid to ( | ||
| dataState.data.repaymentSchedule.periods?.count { it.complete == false } | ||
| ?.toString() ?: "N/A" | ||
| ), | ||
| Res.string.total_installments to dataState.data.termFrequency.toString(), | ||
| Res.string.installment_paid to "0", | ||
| Res.string.total_installments to state.noOfRepayments.toString(), | ||
| ) |
There was a problem hiding this comment.
Bug: state.repaymentSchedule.totalPrincipalPaid reads stale state, not the fresh response.
On line 930, state.repaymentSchedule still holds the previous value because the state update assigning dataState.data happens later at line 942. On the first invocation, this will be RepaymentSchedule() defaults (likely null or 0). Use dataState.data instead:
🐛 Proposed fix
val schedulerDetails = mapOf(
Res.string.account_number to (state.accountNo),
Res.string.disbursement_date to state.expectedDisbursementDate.ifEmpty { "N/A" },
Res.string.principle_paid_off to CurrencyFormatter.format(
- balance = state.repaymentSchedule.totalPrincipalPaid,
+ balance = dataState.data.totalPrincipalPaid,
currencyCode = dataState.data.currency?.code ?: "N/A",
maximumFractionDigits = dataState.data.currency?.decimalPlaces ?: 0,
),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountViewModel.kt`
around lines 926 - 936, The schedulerDetails map is using stale
state.repaymentSchedule (which is updated later) causing totalPrincipalPaid to
be wrong; update the map to read totalPrincipalPaid from the fresh response
dataState.data.repaymentSchedule (e.g., use
dataState.data.repaymentSchedule.totalPrincipalPaid) and keep the existing
null-safety for currency code and decimalPlaces when calling
CurrencyFormatter.format; ensure you reference dataState.data rather than state
for repayment values inside the schedulerDetails construction in
NewLoanAccountViewModel.kt.
| @OptIn(ExperimentalTime::class) | ||
| constructor( | ||
| val launchEffectKey: Int? = null, | ||
| val accountNo: String = "", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n 'navigateToNewLoanAccountRoute|NewLoanAccountRoute\(' --type=kt -C 3Repository: openMF/android-client
Length of output: 91
🏁 Script executed:
rg -n 'navigateToNewLoanAccountRoute|NewLoanAccountRoute\(' -C 3Repository: openMF/android-client
Length of output: 4282
🏁 Script executed:
# View the ViewModel state definition around line 1025
sed -n '1020,1030p' feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountViewModel.kt
# Find all actual invocation sites of navigateToNewLoanAccountRoute with arguments
rg 'navigateToNewLoanAccountRoute\s*\(' -A 2Repository: openMF/android-client
Length of output: 908
🏁 Script executed:
# Find where onNavigateApplyLoanAccount callback is used
rg 'onNavigateApplyLoanAccount' -B 3 -A 3
# Also search for direct calls to navigateToNewLoanAccountRoute with actual parameter passing
rg 'navigateToNewLoanAccountRoute\s*\([^)]*\)' -B 2 -A 2Repository: openMF/android-client
Length of output: 6667
🏁 Script executed:
# Find ClientApplyNewApplicationsScreen state definition
rg 'data class.*ClientApplyNewApplicationsState|ClientApplyNewApplicationsState\(' -A 10 | head -50
# Also check the full invocation context around the navigation call
rg -B 5 -A 2 'onNavigateApplyLoanAccount\(' feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientApplyNewApplications/ClientApplyNewApplicationsScreen.ktRepository: openMF/android-client
Length of output: 4022
🏁 Script executed:
# Find ClientApplyNewApplicationsRoute definition
rg 'data class ClientApplyNewApplicationsRoute|@Serializable.*ClientApplyNewApplicationsRoute' -A 5
# Also search for all places that navigate to ClientApplyNewApplicationsRoute
rg 'navigateToClientApplyNewApplicationsRoute|ClientApplyNewApplicationsRoute\(' -B 2 -A 2Repository: openMF/android-client
Length of output: 47
🏁 Script executed:
# Find files containing ClientApplyNewApplicationsRoute
fd 'ClientApplyNewApplications' --type f
# Also search for where clientApplyNewApplicationRoute is navigated to
rg 'clientApplyNewApplicationRoute\(' -B 2 -A 2Repository: openMF/android-client
Length of output: 1757
🏁 Script executed:
# Find and read the ClientApplyNewApplicationRoute.kt file
cat feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientApplyNewApplications/ClientApplyNewApplicationRoute.kt | head -50Repository: openMF/android-client
Length of output: 1885
🏁 Script executed:
# Find all places that navigate to ClientApplyNewApplicationScreen
rg 'navigateToClientApplyNewApplicationScreen' -B 3 -A 3Repository: openMF/android-client
Length of output: 3461
🏁 Script executed:
# Find where navigateToApplyNewApplication is invoked with arguments
rg 'navigateToApplyNewApplication' -B 2 -A 2 | grep -v "^feature/client/src/commonMain/kotlin/com/mifos/feature/client/navigation/ClientNavigation.kt"
# Also check ClientScreen for the callback invocation
rg 'navigateToApplyNewApplication\(' feature/client/src/commonMain/kotlin/com/mifos/feature/client -B 2 -A 2Repository: openMF/android-client
Length of output: 4212
🏁 Script executed:
# Get the complete invocation with all parameters
rg 'ApplyNewApplication' -A 5 feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientDetailsProfile/ClientProfileDetailsScreen.ktRepository: openMF/android-client
Length of output: 739
Confirm that accountNo defaults to empty string when client has no account number.
The review concern is valid. The accountNo can indeed be empty when navigating to NewLoanAccountRoute. In ClientProfileDetailsScreen, when the "Apply New Application" action is triggered, it passes state.client?.accountNo ?: "" to the navigation chain. If the client object lacks an accountNo, an empty string is passed through navigateToClientApplyNewApplicationScreen → ClientApplyNewApplicationsState → navigateToNewLoanAccountRoute, ultimately reaching the ViewModel state as an empty string.
Clarify whether this behavior is intentional. If the schedule summary should always display a valid account number, consider adding validation or a fallback mechanism at the point where the client's account number is accessed in ClientProfileDetailsScreen.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountViewModel.kt`
at line 1025, The current NewLoanAccountViewModel state property accountNo
defaults to an empty string and can be passed through from
ClientProfileDetailsScreen (via navigateToClientApplyNewApplicationScreen →
ClientApplyNewApplicationsState → navigateToNewLoanAccountRoute), which means
the schedule summary may show no account number; decide whether empty is
acceptable or enforce a fallback/validation: either change the flow to pass a
non-empty fallback (e.g., "N/A" or generate a placeholder) at the caller
(ClientProfileDetailsScreen) before calling
navigateToClientApplyNewApplicationScreen, or add validation in
NewLoanAccountViewModel (or the code that renders the schedule summary) to
detect accountNo == "" and substitute a clear fallback or show a
validation/error state; update the relevant places (ClientProfileDetailsScreen,
navigateToClientApplyNewApplicationScreen, ClientApplyNewApplicationsState,
navigateToNewLoanAccountRoute, and NewLoanAccountViewModel) so the chosen
behavior is consistent.
| * Used to preview the schedule before submitting the loan application. | ||
| */ | ||
| @POST(APIEndPoint.CREATE_LOANS_ACCOUNTS + "?command=calculateLoanSchedule") | ||
| fun calculateLoanSchedule(@Body loansPayload: LoansPayload?): Flow<LoanWithAssociationsEntity> |
There was a problem hiding this comment.
it should have reponse type instead the of HttpResponse
There was a problem hiding this comment.
So sir how can we handle this error msg
{
"developerMessage": "The request was invalid. This typically will happen due to validation errors which are provided.",
"httpStatusCode": "400",
"defaultUserMessage": "Validation errors exist.",
"userMessageGlobalisationCode": "validation.msg.validation.errors.exist",
"errors": [
{
"defaultUserMessage": "The parameter `numberOfRepayments` must be between 2 and 5.",
"parameterName": "numberOfRepayments",
"developerMessage": "The parameter `numberOfRepayments` must be between 2 and 5.",
"userMessageGlobalisationCode": "validation.msg.loan.numberOfRepayments.is.not.within.expected.range",
"args": [
{
"value": 60
},
{
"value": 2
},
{
"value": 5
}
]
}
]
}which is I implemented like this in datamanager
fun calculateLoanSchedule(loansPayload: LoansPayload): Flow<RepaymentSchedule> {
return mBaseApiManager.loanService.calculateLoanSchedule(loansPayload).map { response ->
if (!response.status.isSuccess()) {
val errorMessage = extractErrorMessage(response)
throw IllegalStateException(errorMessage)
}
Json { ignoreUnknownKeys = true }.decodeFromString<RepaymentSchedule>(response.bodyAsText())
}
}There was a problem hiding this comment.
Look other repositories, we are already doing it
There was a problem hiding this comment.
Sir this same approach use everywhere
WhatsApp.Video.2026-02-21.at.6.32.34.PM.mp4 |
2 participants
Summary
Fixes MIFOSAC-705 - Repayment calendar for a new Loan Application shows wrong installment status and wrong dates.
Problem
When creating a new Loan Application, the Schedule (Calendar) step was showing:
Root Cause
In
NewLoanAccountViewModel.kt, therepaymentScheduler()function was callingsyncLoanById(state.clientId)which fetched an existing loan by client ID, instead of calculating a preview based on the form inputs the user just entered.Solution
POST /loans?command=calculateLoanScheduleto calculate loan schedule preview without creating the loanCalculateLoanScheduleUseCaseto handle schedule calculationrepaymentScheduler()to buildLoansPayloadfrom current form state and use the new APIChanges
LoanService.ktcalculateLoanScheduleendpointDataManagerLoan.ktLoanAccountRepository.ktLoanAccountRepositoryImp.ktCalculateLoanScheduleUseCase.ktUseCaseModule.ktNewLoanAccountViewModel.ktrepaymentScheduler()logicTest plan
Summary by CodeRabbit
New Features
Refactor
Model