feat(loan): implement loan reschedules screen with reschedule flow - #2651
Merged
biplab1 merged 4 commits intoMar 25, 2026
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a complete Loan Reschedules feature: new serializable models, network endpoints and DataManagerLoan methods, repository and DI binding, Compose UIs and ViewModels for listing and creating reschedules, navigation routes, design tokens, an API endpoint constant, and localized strings. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as "Compose UI"
participant VM as "ViewModel"
participant Repo as "Repository"
participant DM as "DataManagerLoan"
participant API as "LoanService / Remote API"
UI->>VM: user action (fetch / submit / approve / delete)
VM->>Repo: call get/submit/approve/delete
Repo->>DM: delegate to DataManagerLoan
DM->>API: HTTP request to reschedule endpoint
API-->>DM: HTTP response
DM-->>Repo: emit Flow / return result
Repo-->>VM: DataState (Loading/Success/Error)
VM-->>UI: update UI state / show dialogs / navigate
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: 4
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/loanAccountProfile/LoanAccountProfileScreen.kt (1)
117-125:⚠️ Potential issue | 🟡 MinorGuard
loanIdbefore navigation to avoid invalid route arguments.Line 117 defaults to
-1, and Line 124 forwards it directly. If this event is emitted before account load completes, the reschedules route can receive an invalid ID.Suggested fix
- val loanId = state.loanAccount?.id ?: -1 + val loanId = state.loanAccount?.id ?: return@EventsEffect🤖 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/loanAccountProfile/LoanAccountProfileScreen.kt` around lines 117 - 125, The code obtains loanId using state.loanAccount?.id ?: -1 and immediately calls navigation helpers (navigateToRepaymentSchedule, navigateToTransactions, navigateToCharges, navigateToDocuments, navigateToReschedules, navigateToNotes) which can forward an invalid -1 id; guard the navigation by checking the loan id or loanAccount is present (e.g., require a non-negative/positive id or non-null state.loanAccount) before calling any navigateTo... function and otherwise ignore the event or show a loading/error state so no invalid route argument is passed.
🧹 Nitpick comments (6)
core/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/reschedules/RescheduleStatus.kt (1)
14-22: Consider adding default values for defensive deserialization.All fields are non-nullable without defaults, which means JSON deserialization will fail if the backend omits any field. Other model classes in this PR (e.g.,
RescheduleTimeline,LoanRescheduleResponse) use nullable fields with defaults for resilience.Additionally, note that there's a separate
enum class RescheduleStatusinLoanReschedulesViewModel.kt(used for UI state mapping). While this works, the identical naming could cause confusion for future maintainers.♻️ Optional: Add defaults for defensive deserialization
`@Serializable` data class RescheduleStatus( - val id: Int, - val code: String, - val value: String, - val pendingApproval: Boolean, - val approved: Boolean, - val rejected: Boolean, + val id: Int = 0, + val code: String = "", + val value: String = "", + val pendingApproval: Boolean = false, + val approved: Boolean = false, + val rejected: Boolean = false, )🤖 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/reschedules/RescheduleStatus.kt` around lines 14 - 22, The RescheduleStatus data class currently has all non-nullable properties without defaults which will break deserialization if backend omits fields; update the data class RescheduleStatus to use safe defaults (e.g., make String/Int/Boolean fields nullable or supply sensible default values) so JSON parsing is defensive, and ensure the `@Serializable` constructor reflects those defaults; also consider renaming the UI enum RescheduleStatus in LoanReschedulesViewModel.kt (or vice‑versa) to avoid confusing identical type names for future maintainers.core/network/src/commonMain/kotlin/com/mifos/core/network/services/LoanService.kt (1)
194-198: Semantic mismatch: method nameddeleteLoanRescheduleusesrejectcommand.The method name implies deletion but the endpoint uses
?command=reject. Consider renaming torejectLoanReschedulefor API clarity and to match the actual operation being performed.Suggested rename
- `@POST`(APIEndPoint.RESCHEDULE_LOANS + "/{scheduleId}?command=reject") - suspend fun deleteLoanReschedule( + `@POST`(APIEndPoint.RESCHEDULE_LOANS + "/{scheduleId}?command=reject") + suspend fun rejectLoanReschedule( `@Path`("scheduleId") scheduleId: Int, `@Body` request: LoanRescheduleRejectionRequest, ): HttpResponseThis would require corresponding changes in
DataManagerLoanandLoanReschedulesRepositoryImpl.🤖 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 194 - 198, The method deleteLoanReschedule is semantically misnamed because it calls the endpoint with ?command=reject; rename the function deleteLoanReschedule to rejectLoanReschedule (and update its usages) so the method name matches the API operation, update the Retrofit annotation if needed (keep `@POST`(APIEndPoint.RESCHEDULE_LOANS + "/{scheduleId}?command=reject")), and propagate the rename to callers in DataManagerLoan and LoanReschedulesRepositoryImpl to ensure compilation and clarity.feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/createLoanReschedules/LoanRescheduleFormScreen.kt (1)
267-267: DatePickerState not initialized with existing value.The
rememberDatePickerState()is created without an initial selection. When the user opens the picker, it won't show the currently selected date. Consider initializing with the existing value if one is set.Initialize date picker with existing value
+import kotlinx.datetime.LocalDate +import kotlinx.datetime.TimeZone +import kotlinx.datetime.atStartOfDayIn `@OptIn`(ExperimentalMaterial3Api::class) `@Composable` private fun RescheduleDatePickerField( value: String, label: String, onDateSelected: (String) -> Unit, modifier: Modifier = Modifier, ) { var showDialog by remember { mutableStateOf(false) } - val datePickerState = rememberDatePickerState() + val initialMillis = remember(value) { + if (value.isNotBlank()) { + // Parse the value back to millis if needed, or pass initial millis as parameter + null // Could parse "dd MMMM yyyy" format back to millis + } else null + } + val datePickerState = rememberDatePickerState(initialSelectedDateMillis = initialMillis)Note: This requires parsing the formatted date string back to milliseconds, which may need additional utility support.
🤖 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/createLoanReschedules/LoanRescheduleFormScreen.kt` at line 267, The DatePickerState created in LoanRescheduleFormScreen via rememberDatePickerState() lacks an initial selection so the picker won't reflect the currently chosen date; update the call to rememberDatePickerState(initialSelectedDateMillis = ...) using the existing selected date value (convert the formatted date string back to epoch milliseconds first), e.g. add or reuse a utility like parseDateStringToMillis(dateString, format) to produce the Long and pass it into rememberDatePickerState; ensure null/parse-failure handling (fallback to null) so the picker still opens if no valid date exists.core/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerLoan.kt (1)
412-425: Method naming may cause confusion:deleteLoanRescheduleactually performs rejection.The method name suggests deletion, but it sends a
LoanRescheduleRejectionRequestwithrejectedOnDateto therejectcommand endpoint. Consider renaming for clarity.Suggested rename for clarity
- suspend fun deleteLoanReschedule(scheduleId: Int) { + suspend fun rejectLoanReschedule(scheduleId: Int) { val today = ApiDateFormatter.formatForApi(Clock.System.now().toEpochMilliseconds()) - val response = mBaseApiManager.loanService.deleteLoanReschedule( + val response = mBaseApiManager.loanService.rejectLoanReschedule( scheduleId = scheduleId, request = LoanRescheduleRejectionRequest(🤖 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 412 - 425, The method deleteLoanReschedule currently sends a LoanRescheduleRejectionRequest to the loanService (rejection flow) which is misleading; rename the function deleteLoanReschedule to rejectLoanReschedule (or rejectLoanRescheduleById) and update all call sites and tests to use the new name, keeping the body (the LoanRescheduleRejectionRequest creation and the response error handling via extractErrorMessage) unchanged; ensure any public API/docs and usage of mBaseApiManager.loanService.deleteLoanReschedule are updated to reflect the clearer intent.feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanReschedules/LoanReschedulesScreen.kt (1)
421-436: Consider adding semantic accessibility for action buttons.The clickable boxes lack proper accessibility semantics. While
contentDescriptionis set on the icons, the clickable area itself doesn't indicate the action to screen readers.Accessibility improvement
Box( modifier = Modifier .size(DesignToken.sizes.iconLarge) .clip(KptTheme.shapes.extraSmall) .background(KptTheme.colorScheme.error) - .clickable { onDeleteClick() }, + .clickable( + onClickLabel = stringResource(Res.string.feature_loan_reschedule_delete_confirm), + ) { onDeleteClick() }, contentAlignment = Alignment.Center, ) {🤖 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/loanReschedules/LoanReschedulesScreen.kt` around lines 421 - 436, The clickable Box for delete (inside the isPending branch) lacks accessibility semantics for screen readers; add a semantics modifier to the Box modifier chain (e.g., Modifier.size(...).clip(...).background(...).semantics { contentDescription = stringResource(Res.string.feature_loan_reschedule_delete_confirm); role = Role.Button } .clickable { onDeleteClick() }) so the clickable area exposes the same descriptive label and Role.Button to accessibility services; apply the same pattern to any other action Box/Icon combinations (reference: isPending, Box, onDeleteClick, Icon, MifosIcons.Delete, feature_loan_reschedule_delete_confirm).feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/createLoanReschedules/LoanRescheduleFormViewModel.kt (1)
196-215: Default dates are captured at state instantiation time.The default values for
rescheduleFromDateandsubmittedOnDateuseClock.System.now().toEpochMilliseconds()which is evaluated whenLoanRescheduleFormUiStateis first instantiated. If the user navigates to this screen and leaves it open for a while before submitting, the date will reflect when they opened the screen, not the current date.This is typically acceptable behavior for form pre-population, but worth noting if exact submission time is critical.
🤖 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/createLoanReschedules/LoanRescheduleFormViewModel.kt` around lines 196 - 215, LoanRescheduleFormUiState currently evaluates Clock.System.now() when the data class is instantiated (rescheduleFromDate and submittedOnDate), which freezes the default timestamp at screen-open; change this so the current date is computed at use/submit instead: remove the direct ApiDateFormatter.formatForApi(Clock.System.now().toEpochMilliseconds()) defaults from LoanRescheduleFormUiState (e.g., make those fields nullable or empty strings) and compute ApiDateFormatter.formatForApi(Clock.System.now().toEpochMilliseconds()) in LoanRescheduleFormViewModel (e.g., in init or right before submit) or via a lazy/provider function so the timestamp reflects the actual submission time. Ensure references to LoanRescheduleFormUiState.rescheduleFromDate, LoanRescheduleFormUiState.submittedOnDate and LoanRescheduleFormViewModel initialization/submit logic are updated accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/createLoanReschedules/LoanRescheduleFormScreen.kt`:
- Around line 258-296: The RescheduleDatePickerField Composable declares a
modifier parameter but doesn’t apply it; update RescheduleDatePickerField to
pass the incoming modifier to the root element(s) (e.g., apply modifier instead
of Modifier.fillMaxWidth() on the MifosDatePickerTextField or compose it with
the DatePickerDialog container) so the caller’s layout hints are honored; locate
the RescheduleDatePickerField function and replace hardcoded Modifier usages
with the provided modifier (or modifier.then(...)) where appropriate.
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/createLoanReschedules/LoanRescheduleFormViewModel.kt`:
- Around line 110-122: fetchRescheduleTemplate currently only processes
DataState.Success from repository.getLoanRescheduleTemplate() and ignores
Loading and Error; update the collect block to handle DataState.Loading by
updating mutableStateFlow (e.g., set an isLoading flag) and handle
DataState.Error by updating mutableStateFlow with an error message/state and
optionally logging the error so the UI can show feedback, while keeping the
existing update on DataState.Success to set reasons sorted by name; make changes
inside fetchRescheduleTemplate and the collect lambda, touching DataState
matching and mutableStateFlow.update (and add/ensure isLoading/error fields on
the view state if not present).
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanReschedules/LoanReschedulesScreen.kt`:
- Around line 269-278: The empty-state Text is currently centered in the outer
Box which can overlap the Column’s title and spacer; update
LoanReschedulesScreen so the Column renders either the history table or the
empty message: inside the Column (where the title and Spacer are rendered)
replace the table block with a conditional on state.history.isNotEmpty() to
render the existing table content, and in the else branch render a Box(modifier
= Modifier.fillMaxWidth().weight(1f), contentAlignment = Alignment.Center) {
Text(...) } so the empty message is vertically centered without layering over
the title/spacer.
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanReschedules/LoanReschedulesScreenRoute.kt`:
- Around line 30-35: You're calling the serializable data class
LoanReschedulesScreenRoute(...) inside the composable block instead of the
`@Composable` function with the same name; replace that call with the composable
function LoanReschedulesScreenRoute(navController = navController, navigateBack
= onBackPressed) (or qualify/import the composable from
LoanReschedulesScreen.kt), or alternatively rename the data class to avoid the
collision so the composable function (signature:
LoanReschedulesScreenRoute(navController: NavController, viewModel:
LoanReschedulesViewModel = koinViewModel(), navigateBack: () -> Unit)) is
invoked.
---
Outside diff comments:
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/LoanAccountProfileScreen.kt`:
- Around line 117-125: The code obtains loanId using state.loanAccount?.id ?: -1
and immediately calls navigation helpers (navigateToRepaymentSchedule,
navigateToTransactions, navigateToCharges, navigateToDocuments,
navigateToReschedules, navigateToNotes) which can forward an invalid -1 id;
guard the navigation by checking the loan id or loanAccount is present (e.g.,
require a non-negative/positive id or non-null state.loanAccount) before calling
any navigateTo... function and otherwise ignore the event or show a
loading/error state so no invalid route argument is passed.
---
Nitpick comments:
In
`@core/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/reschedules/RescheduleStatus.kt`:
- Around line 14-22: The RescheduleStatus data class currently has all
non-nullable properties without defaults which will break deserialization if
backend omits fields; update the data class RescheduleStatus to use safe
defaults (e.g., make String/Int/Boolean fields nullable or supply sensible
default values) so JSON parsing is defensive, and ensure the `@Serializable`
constructor reflects those defaults; also consider renaming the UI enum
RescheduleStatus in LoanReschedulesViewModel.kt (or vice‑versa) to avoid
confusing identical type names for future maintainers.
In
`@core/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerLoan.kt`:
- Around line 412-425: The method deleteLoanReschedule currently sends a
LoanRescheduleRejectionRequest to the loanService (rejection flow) which is
misleading; rename the function deleteLoanReschedule to rejectLoanReschedule (or
rejectLoanRescheduleById) and update all call sites and tests to use the new
name, keeping the body (the LoanRescheduleRejectionRequest creation and the
response error handling via extractErrorMessage) unchanged; ensure any public
API/docs and usage of mBaseApiManager.loanService.deleteLoanReschedule are
updated to reflect the clearer intent.
In
`@core/network/src/commonMain/kotlin/com/mifos/core/network/services/LoanService.kt`:
- Around line 194-198: The method deleteLoanReschedule is semantically misnamed
because it calls the endpoint with ?command=reject; rename the function
deleteLoanReschedule to rejectLoanReschedule (and update its usages) so the
method name matches the API operation, update the Retrofit annotation if needed
(keep `@POST`(APIEndPoint.RESCHEDULE_LOANS + "/{scheduleId}?command=reject")), and
propagate the rename to callers in DataManagerLoan and
LoanReschedulesRepositoryImpl to ensure compilation and clarity.
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/createLoanReschedules/LoanRescheduleFormScreen.kt`:
- Line 267: The DatePickerState created in LoanRescheduleFormScreen via
rememberDatePickerState() lacks an initial selection so the picker won't reflect
the currently chosen date; update the call to
rememberDatePickerState(initialSelectedDateMillis = ...) using the existing
selected date value (convert the formatted date string back to epoch
milliseconds first), e.g. add or reuse a utility like
parseDateStringToMillis(dateString, format) to produce the Long and pass it into
rememberDatePickerState; ensure null/parse-failure handling (fallback to null)
so the picker still opens if no valid date exists.
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/createLoanReschedules/LoanRescheduleFormViewModel.kt`:
- Around line 196-215: LoanRescheduleFormUiState currently evaluates
Clock.System.now() when the data class is instantiated (rescheduleFromDate and
submittedOnDate), which freezes the default timestamp at screen-open; change
this so the current date is computed at use/submit instead: remove the direct
ApiDateFormatter.formatForApi(Clock.System.now().toEpochMilliseconds()) defaults
from LoanRescheduleFormUiState (e.g., make those fields nullable or empty
strings) and compute
ApiDateFormatter.formatForApi(Clock.System.now().toEpochMilliseconds()) in
LoanRescheduleFormViewModel (e.g., in init or right before submit) or via a
lazy/provider function so the timestamp reflects the actual submission time.
Ensure references to LoanRescheduleFormUiState.rescheduleFromDate,
LoanRescheduleFormUiState.submittedOnDate and LoanRescheduleFormViewModel
initialization/submit logic are updated accordingly.
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanReschedules/LoanReschedulesScreen.kt`:
- Around line 421-436: The clickable Box for delete (inside the isPending
branch) lacks accessibility semantics for screen readers; add a semantics
modifier to the Box modifier chain (e.g.,
Modifier.size(...).clip(...).background(...).semantics { contentDescription =
stringResource(Res.string.feature_loan_reschedule_delete_confirm); role =
Role.Button } .clickable { onDeleteClick() }) so the clickable area exposes the
same descriptive label and Role.Button to accessibility services; apply the same
pattern to any other action Box/Icon combinations (reference: isPending, Box,
onDeleteClick, Icon, MifosIcons.Delete, feature_loan_reschedule_delete_confirm).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9bc12320-3e3d-4ef7-a8be-20e9eee15cc6
📒 Files selected for processing (26)
core/data/src/commonMain/kotlin/com/mifos/core/data/di/RepositoryModule.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repository/LoanReschedulesRepository.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/LoanReschedulesRepositoryImpl.ktcore/database/src/commonMain/kotlin/com/mifos/room/basemodel/APIEndPoint.ktcore/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/theme/DesignToken.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/reschedules/LoanRescheduleApprovalRequest.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/reschedules/LoanRescheduleRejectionRequest.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/reschedules/LoanRescheduleRequest.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/reschedules/LoanRescheduleResponse.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/reschedules/LoanRescheduleTemplate.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/reschedules/RescheduleReasonOption.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/reschedules/RescheduleStatus.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/reschedules/RescheduleTimeline.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerLoan.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/services/LoanService.ktfeature/loan/src/commonMain/composeResources/values/strings.xmlfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/createLoanReschedules/LoanRescheduleFormScreen.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/createLoanReschedules/LoanRescheduleFormViewModel.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/createLoanReschedules/LoanReschedulesFormScreenRoute.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/di/LoanModule.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/LoanAccountProfileNavigation.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/LoanAccountProfileScreen.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanReschedules/LoanReschedulesScreen.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanReschedules/LoanReschedulesScreenRoute.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanReschedules/LoanReschedulesViewModel.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/navigation/LoanNavigation.kt
kartikey004
force-pushed
the
feature/loan-reschedules
branch
2 times, most recently
from
March 15, 2026 11:50
f57c595 to
ff8b18e
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
feature/loan/src/commonMain/composeResources/values/strings.xml (1)
434-445: Reuse the shared action/result strings here.
Submit,Cancel,Success, andErroralready exist earlier in this file (feature_loan_submit,feature_loan_cancel,feature_loan_success_title,feature_loan_error_title). Reusing those keeps terminology aligned and avoids extra resource IDs to translate and maintain.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/loan/src/commonMain/composeResources/values/strings.xml` around lines 434 - 445, Replace duplicated generic action/result strings with the existing shared resource IDs: change feature_loan_reschedule_submit to reuse feature_loan_submit, feature_loan_reschedule_cancel to reuse feature_loan_cancel, feature_loan_reschedule_success_title to reuse feature_loan_success_title, and feature_loan_reschedule_failure_title to reuse feature_loan_error_title; keep the reschedule-specific keys (feature_loan_reschedule_label_reason, feature_loan_reschedule_label_status, feature_loan_reschedule_label_from_date, feature_loan_reschedule_label_na, feature_loan_must_select_reason, feature_loan_reschedule_date_required, feature_loan_reschedule_go_back) as-is. Ensure the duplicated string values are replaced by references to the existing IDs so translations and terminology remain consistent.
🤖 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/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/LoanReschedulesRepositoryImpl.kt`:
- Around line 35-59: The try/catch blocks in submitLoanReschedule,
approveReschedule, and deleteReschedule are catching all Exception types and
thus swallowing kotlinx.coroutines.CancellationException; update each catch to
rethrow CancellationException explicitly (i.e., if (e is CancellationException)
throw e) before returning DataState.Error for other exceptions so coroutine
cancellation propagates correctly; locate these in LoanReschedulesRepositoryImpl
where dataManagerLoan.submitLoanReschedule,
dataManagerLoan.approveLoanReschedule, and dataManagerLoan.deleteLoanReschedule
are invoked and apply the rethrow pattern.
In
`@core/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/reschedules/LoanRescheduleRequest.kt`:
- Around line 22-27: Change the optional reschedule fields to nullable with null
defaults so disabled form toggles aren't serialized as empty strings: update the
properties graceOnPrincipal, graceOnInterest, extraTerms, adjustedDueDate,
newInterestRate, and rescheduleReasonComment in LoanRescheduleRequest to type
String? and give each a default value of null (following the existing pattern
used for waivePenalties), ensuring the data class constructor/signature reflects
these nullable defaults.
In
`@core/network/src/commonMain/kotlin/com/mifos/core/network/services/LoanService.kt`:
- Around line 194-198: The function deleteLoanReschedule is misnamed for an
endpoint that performs a "reject" action; rename the service method
deleteLoanReschedule to rejectLoanReschedule, keeping the signature (use
`@POST`(APIEndPoint.RESCHEDULE_LOANS + "/{scheduleId}?command=reject") suspend fun
rejectLoanReschedule(`@Path`("scheduleId") scheduleId: Int, `@Body` request:
LoanRescheduleRejectionRequest): HttpResponse), then update all call sites (data
manager, repository methods that reference deleteLoanReschedule), any DTO
mappings that pass LoanRescheduleRejectionRequest, and related UI strings/labels
that currently say "delete" to use "reject" to keep terminology consistent
across the stack; also update any tests or mocks referencing
deleteLoanReschedule.
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/createLoanReschedules/LoanRescheduleFormViewModel.kt`:
- Around line 102-103: The retry action currently calls submitReschedule()
directly which can crash on the forced unwrap state.selectedReasonId!!; update
the LoanRescheduleFormViewModel action handler so
LoanRescheduleFormAction.OnRetrySubmitClick invokes validateAndSubmit() (same
path as OnSubmitClicked) OR add a defensive null-check inside
submitReschedule(): verify state.selectedReasonId is non-null (return or emit an
error state if null) before using it. Ensure changes reference
validateAndSubmit(), submitReschedule(), and state.selectedReasonId and preserve
existing error/reporting behavior when validation fails.
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanReschedules/LoanReschedulesViewModel.kt`:
- Around line 159-166: The current String?.toRescheduleStatus() uses fragile
text matching; replace it with an extension that reads the backend boolean flags
instead. Create/replace toRescheduleStatus so it is defined on the canonical
status object/type (the item.statusEnum type) and return
RescheduleStatus.PENDING when pendingApproval==true, APPROVED when
approved==true, REJECTED when rejected==true, otherwise UNKNOWN; update call
sites (e.g., where LoanReschedulesScreen.kt uses
item.statusEnum.toRescheduleStatus()) to call the new extension on the
statusEnum object rather than passing raw text.
- Around line 58-77: fetchRescheduleHistory() currently launches a new collector
each call causing overlapping collectors and racey UI updates; fix by tracking
and cancelling the previous collector job before starting a new one: add a
private var (e.g., historyJob: Job?) in the ViewModel, call historyJob?.cancel()
at the start of fetchRescheduleHistory(), then assign historyJob =
viewModelScope.launch { repository.getLoanReschedules(route.loanId).collect {
... mutableStateFlow.update { ... } } } so only one active collector updates
state at a time (references: fetchRescheduleHistory,
repository.getLoanReschedules, mutableStateFlow.update).
---
Nitpick comments:
In `@feature/loan/src/commonMain/composeResources/values/strings.xml`:
- Around line 434-445: Replace duplicated generic action/result strings with the
existing shared resource IDs: change feature_loan_reschedule_submit to reuse
feature_loan_submit, feature_loan_reschedule_cancel to reuse
feature_loan_cancel, feature_loan_reschedule_success_title to reuse
feature_loan_success_title, and feature_loan_reschedule_failure_title to reuse
feature_loan_error_title; keep the reschedule-specific keys
(feature_loan_reschedule_label_reason, feature_loan_reschedule_label_status,
feature_loan_reschedule_label_from_date, feature_loan_reschedule_label_na,
feature_loan_must_select_reason, feature_loan_reschedule_date_required,
feature_loan_reschedule_go_back) as-is. Ensure the duplicated string values are
replaced by references to the existing IDs so translations and terminology
remain consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b442239c-a9fc-42b9-915c-d66ad79325b7
📒 Files selected for processing (26)
core/data/src/commonMain/kotlin/com/mifos/core/data/di/RepositoryModule.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repository/LoanReschedulesRepository.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/LoanReschedulesRepositoryImpl.ktcore/database/src/commonMain/kotlin/com/mifos/room/basemodel/APIEndPoint.ktcore/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/theme/DesignToken.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/reschedules/LoanRescheduleApprovalRequest.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/reschedules/LoanRescheduleRejectionRequest.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/reschedules/LoanRescheduleRequest.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/reschedules/LoanRescheduleResponse.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/reschedules/LoanRescheduleTemplate.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/reschedules/RescheduleReasonOption.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/reschedules/RescheduleStatus.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/reschedules/RescheduleTimeline.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerLoan.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/services/LoanService.ktfeature/loan/src/commonMain/composeResources/values/strings.xmlfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/createLoanReschedules/LoanRescheduleFormScreen.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/createLoanReschedules/LoanRescheduleFormViewModel.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/createLoanReschedules/LoanReschedulesFormScreenRoute.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/di/LoanModule.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/LoanAccountProfileNavigation.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/LoanAccountProfileScreen.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanReschedules/LoanReschedulesScreen.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanReschedules/LoanReschedulesScreenRoute.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanReschedules/LoanReschedulesViewModel.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/navigation/LoanNavigation.kt
🚧 Files skipped from review as they are similar to previous changes (12)
- core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/theme/DesignToken.kt
- feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/LoanAccountProfileNavigation.kt
- core/database/src/commonMain/kotlin/com/mifos/room/basemodel/APIEndPoint.kt
- feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/createLoanReschedules/LoanReschedulesFormScreenRoute.kt
- core/data/src/commonMain/kotlin/com/mifos/core/data/di/RepositoryModule.kt
- feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/LoanAccountProfileScreen.kt
- core/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/reschedules/RescheduleReasonOption.kt
- feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/createLoanReschedules/LoanRescheduleFormScreen.kt
- feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/navigation/LoanNavigation.kt
- feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/di/LoanModule.kt
- core/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/reschedules/RescheduleStatus.kt
- core/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/reschedules/LoanRescheduleResponse.kt
There was a problem hiding this comment.
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/loanAccountProfile/LoanAccountProfileScreen.kt (1)
117-125:⚠️ Potential issue | 🟠 MajorGuard against invalid
loanIdbefore detail navigation.At Line 117, falling back to
-1allows Line 124 to navigate to reschedules with an invalid ID. This can open downstream screens in a broken state (or trigger invalid API calls). Prefer returning early whenloanAccountis null.🔧 Proposed fix
- val loanId = state.loanAccount?.id ?: -1 + val loanId = state.loanAccount?.id ?: return@EventsEffect🤖 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/loanAccountProfile/LoanAccountProfileScreen.kt` around lines 117 - 125, The code uses a fallback loanId = state.loanAccount?.id ?: -1 and proceeds to call navigation functions (navigateToRepaymentSchedule, navigateToTransactions, navigateToCharges, navigateToDocuments, navigateToReschedules, navigateToNotes) even when loanAccount is null; change the logic to guard early: check state.loanAccount (or that loanId is non-null/positive) and return early (do not call any navigateTo... methods) when the loan ID is invalid, otherwise proceed to call the appropriate navigateTo... function with the valid id.
♻️ Duplicate comments (1)
core/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/reschedules/LoanRescheduleRequest.kt (1)
22-27:⚠️ Potential issue | 🟠 MajorDon't make every reschedule modifier mandatory.
Fineract documents
graceOnPrincipal,graceOnInterest,extraTerms,newInterestRate, andadjustedDueDateas an OR-set where one is required, andrescheduleReasonCommentas optional; the official examples also send unused modifier fields asnull. Keeping all six as non-nullStrings forces callers to invent placeholder values for flows that only change one term. (fineract.apache.org)At minimum, make these nullable so callers can omit the unused modifiers entirely.
Minimal DTO fix
data class LoanRescheduleRequest( val loanId: Int, val rescheduleFromDate: String, val rescheduleReasonId: Int, val submittedOnDate: String, val dateFormat: String, val locale: String, - val graceOnPrincipal: String, - val graceOnInterest: String, - val extraTerms: String, - val adjustedDueDate: String, - val newInterestRate: String, - val rescheduleReasonComment: String, + val graceOnPrincipal: String? = null, + val graceOnInterest: String? = null, + val extraTerms: String? = null, + val adjustedDueDate: String? = null, + val newInterestRate: String? = null, + val rescheduleReasonComment: String? = null, val waivePenalties: Boolean? = null, )🤖 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/reschedules/LoanRescheduleRequest.kt` around lines 22 - 27, The DTO LoanRescheduleRequest currently declares graceOnPrincipal, graceOnInterest, extraTerms, adjustedDueDate, newInterestRate and rescheduleReasonComment as non-null Strings; change these modifier fields to nullable types (e.g., String?) and make rescheduleReasonComment optional (nullable with no required value) so callers can omit unused modifiers, and add/adjust any constructor/defaults accordingly; also add a simple validation method (or update existing validation) in LoanRescheduleRequest to enforce the OR-set rule (at least one of graceOnPrincipal, graceOnInterest, extraTerms, adjustedDueDate, newInterestRate is non-null) rather than forcing all fields to be provided.
🧹 Nitpick comments (4)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/createLoanReschedules/LoanReschedulesFormScreenRoute.kt (2)
10-10: Minor naming inconsistency: file name uses plural but contents use singular.The file is named
LoanReschedulesFormScreenRoute.kt(plural "Reschedules") while the route class (RescheduleScreenRoute) and functions (navigateToLoanRescheduleFormScreen,loanRescheduleFormScreen) use singular naming. Consider renaming the file toLoanRescheduleFormScreenRoute.ktfor consistency.🤖 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/createLoanReschedules/LoanReschedulesFormScreenRoute.kt` at line 10, Rename the source file from LoanReschedulesFormScreenRoute.kt to LoanRescheduleFormScreenRoute.kt to match the singular naming used inside; update any references/imports or build references that point to the old filename so the class RescheduleScreenRoute and the functions navigateToLoanRescheduleFormScreen and loanRescheduleFormScreen continue to resolve correctly.
26-36: NavController usage is limited toMifosBreadcrumbNavBar; consider extracting breadcrumb navigation as a callback if feasible.The
navControlleris only passed toMifosBreadcrumbNavBarand isn't used directly elsewhere in the screen. The rest of the navigation logic properly follows the callback pattern (e.g.,navigateBackcallback for back navigation). If the breadcrumb component's navigation behavior can be abstracted as a callback, this would improve consistency with the rest of the screen's architecture. However, since breadcrumb components typically require navigation capabilities for handling item selections, the current approach is acceptable.🤖 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/createLoanReschedules/LoanReschedulesFormScreenRoute.kt` around lines 26 - 36, The NavController is only used for breadcrumb navigation; change the loanRescheduleFormScreen and LoanRescheduleFormScreenRoute wiring to accept a breadcrumb navigation callback instead of a NavController: update the NavGraphBuilder function signature (loanRescheduleFormScreen) to take an onBreadcrumbNavigate: (String) -> Unit (or appropriate type) and pass that into LoanRescheduleFormScreenRoute, and modify LoanRescheduleFormScreenRoute to forward that callback into MifosBreadcrumbNavBar instead of the NavController; if MifosBreadcrumbNavBar currently requires a NavController, refactor it to accept a navigation callback (e.g., onBreadcrumbNavigate) or add an adapter that converts the callback to NavController calls so the screen remains callback-driven and consistent with navigateBack.feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/createLoanReschedules/LoanRescheduleFormScreen.kt (1)
265-274:ApiDateFormatterdoesn't have aparseFromApi()method; consider an alternative approach for initializing the DatePicker.While initializing the date picker with the currently selected date is a good UX improvement,
ApiDateFormatteronly provides formatting methods, not parsing. To implement this, you'd need to parse the date string usingDateHelper.getDateAsList()and convert it back to milliseconds. The implementation complexity may not justify the benefit for this optional enhancement.🤖 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/createLoanReschedules/LoanRescheduleFormScreen.kt` around lines 265 - 274, ApiDateFormatter has no parseFromApi(), so initialize the date picker by parsing the displayed date string with DateHelper.getDateAsList() and converting that list into epoch milliseconds to pass into rememberDatePickerState (or skip pre-selection if parsing fails). Update the block around rememberDatePickerState() / MifosDatePickerTextField: attempt to parse the incoming value via DateHelper.getDateAsList(value) -> build a year/month/day and convert to milliseconds, then call rememberDatePickerState(initialSelectedDateMillis = parsedMillis); if parsing returns null or throws, fall back to the default rememberDatePickerState() to avoid crashes.core/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerLoan.kt (1)
398-425: Add blank line between methods for consistency.The codebase maintains blank lines between method definitions, but there's no blank line between
approveLoanRescheduleanddeleteLoanReschedule.✨ Minor formatting fix
if (!response.status.isSuccess()) { throw IllegalStateException(extractErrorMessage(response)) } } + suspend fun deleteLoanReschedule(scheduleId: Int) {🤖 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 398 - 425, There is no blank line separating the two suspend functions approveLoanReschedule and deleteLoanReschedule; add a single blank line between the closing brace of approveLoanReschedule and the declaration of deleteLoanReschedule so the methods follow the project's formatting convention for spacing between function definitions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/LoanAccountProfileScreen.kt`:
- Around line 117-125: The code uses a fallback loanId = state.loanAccount?.id
?: -1 and proceeds to call navigation functions (navigateToRepaymentSchedule,
navigateToTransactions, navigateToCharges, navigateToDocuments,
navigateToReschedules, navigateToNotes) even when loanAccount is null; change
the logic to guard early: check state.loanAccount (or that loanId is
non-null/positive) and return early (do not call any navigateTo... methods) when
the loan ID is invalid, otherwise proceed to call the appropriate navigateTo...
function with the valid id.
---
Duplicate comments:
In
`@core/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/reschedules/LoanRescheduleRequest.kt`:
- Around line 22-27: The DTO LoanRescheduleRequest currently declares
graceOnPrincipal, graceOnInterest, extraTerms, adjustedDueDate, newInterestRate
and rescheduleReasonComment as non-null Strings; change these modifier fields to
nullable types (e.g., String?) and make rescheduleReasonComment optional
(nullable with no required value) so callers can omit unused modifiers, and
add/adjust any constructor/defaults accordingly; also add a simple validation
method (or update existing validation) in LoanRescheduleRequest to enforce the
OR-set rule (at least one of graceOnPrincipal, graceOnInterest, extraTerms,
adjustedDueDate, newInterestRate is non-null) rather than forcing all fields to
be provided.
---
Nitpick comments:
In
`@core/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerLoan.kt`:
- Around line 398-425: There is no blank line separating the two suspend
functions approveLoanReschedule and deleteLoanReschedule; add a single blank
line between the closing brace of approveLoanReschedule and the declaration of
deleteLoanReschedule so the methods follow the project's formatting convention
for spacing between function definitions.
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/createLoanReschedules/LoanRescheduleFormScreen.kt`:
- Around line 265-274: ApiDateFormatter has no parseFromApi(), so initialize the
date picker by parsing the displayed date string with DateHelper.getDateAsList()
and converting that list into epoch milliseconds to pass into
rememberDatePickerState (or skip pre-selection if parsing fails). Update the
block around rememberDatePickerState() / MifosDatePickerTextField: attempt to
parse the incoming value via DateHelper.getDateAsList(value) -> build a
year/month/day and convert to milliseconds, then call
rememberDatePickerState(initialSelectedDateMillis = parsedMillis); if parsing
returns null or throws, fall back to the default rememberDatePickerState() to
avoid crashes.
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/createLoanReschedules/LoanReschedulesFormScreenRoute.kt`:
- Line 10: Rename the source file from LoanReschedulesFormScreenRoute.kt to
LoanRescheduleFormScreenRoute.kt to match the singular naming used inside;
update any references/imports or build references that point to the old filename
so the class RescheduleScreenRoute and the functions
navigateToLoanRescheduleFormScreen and loanRescheduleFormScreen continue to
resolve correctly.
- Around line 26-36: The NavController is only used for breadcrumb navigation;
change the loanRescheduleFormScreen and LoanRescheduleFormScreenRoute wiring to
accept a breadcrumb navigation callback instead of a NavController: update the
NavGraphBuilder function signature (loanRescheduleFormScreen) to take an
onBreadcrumbNavigate: (String) -> Unit (or appropriate type) and pass that into
LoanRescheduleFormScreenRoute, and modify LoanRescheduleFormScreenRoute to
forward that callback into MifosBreadcrumbNavBar instead of the NavController;
if MifosBreadcrumbNavBar currently requires a NavController, refactor it to
accept a navigation callback (e.g., onBreadcrumbNavigate) or add an adapter that
converts the callback to NavController calls so the screen remains
callback-driven and consistent with navigateBack.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0c0e7853-4b57-421e-8f8d-e7dbd6fe0342
📒 Files selected for processing (26)
core/data/src/commonMain/kotlin/com/mifos/core/data/di/RepositoryModule.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repository/LoanReschedulesRepository.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/LoanReschedulesRepositoryImpl.ktcore/database/src/commonMain/kotlin/com/mifos/room/basemodel/APIEndPoint.ktcore/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/theme/DesignToken.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/reschedules/LoanRescheduleApprovalRequest.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/reschedules/LoanRescheduleRejectionRequest.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/reschedules/LoanRescheduleRequest.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/reschedules/LoanRescheduleResponse.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/reschedules/LoanRescheduleTemplate.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/reschedules/RescheduleReasonOption.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/reschedules/RescheduleStatus.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/reschedules/RescheduleTimeline.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerLoan.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/services/LoanService.ktfeature/loan/src/commonMain/composeResources/values/strings.xmlfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/createLoanReschedules/LoanRescheduleFormScreen.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/createLoanReschedules/LoanRescheduleFormViewModel.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/createLoanReschedules/LoanReschedulesFormScreenRoute.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/di/LoanModule.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/LoanAccountProfileNavigation.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/LoanAccountProfileScreen.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanReschedules/LoanReschedulesScreen.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanReschedules/LoanReschedulesScreenRoute.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanReschedules/LoanReschedulesViewModel.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/navigation/LoanNavigation.kt
✅ Files skipped from review due to trivial changes (1)
- feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/di/LoanModule.kt
🚧 Files skipped from review as they are similar to previous changes (6)
- core/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/reschedules/LoanRescheduleRejectionRequest.kt
- feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/createLoanReschedules/LoanRescheduleFormViewModel.kt
- core/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/reschedules/RescheduleStatus.kt
- core/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/reschedules/RescheduleReasonOption.kt
- core/data/src/commonMain/kotlin/com/mifos/core/data/repository/LoanReschedulesRepository.kt
- core/data/src/commonMain/kotlin/com/mifos/core/data/di/RepositoryModule.kt
kartikey004
force-pushed
the
feature/loan-reschedules
branch
from
March 15, 2026 12:48
ff8b18e to
45ef6cb
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/LoanReschedulesRepositoryImpl.kt`:
- Around line 58-61: deleteReschedule currently calls
dataManagerLoan.rejectLoanReschedule(rescheduleId) (which uses LoanService POST
...?command=reject), so the flow marks the request REJECTED instead of removing
it; either implement a true delete path or rename the API to reflect rejection.
To implement delete: add a deleteLoanReschedule(rescheduleId: Int) method on
dataManagerLoan and a matching LoanService endpoint that issues the correct
DELETE (or backend-supported) call, then update
LoanReschedulesRepositoryImpl.deleteReschedule to call
dataManagerLoan.deleteLoanReschedule; alternatively, rename deleteReschedule to
rejectReschedule (and update all callers/UI) so the method name matches
dataManagerLoan.rejectLoanReschedule and the UI contract is accurate.
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/createLoanReschedules/LoanRescheduleFormScreen.kt`:
- Around line 306-315: The Checkbox is currently non-interactive visually
(checked = selected, onCheckedChange = null) while the Row handles clicks, which
breaks checkbox semantics; either make the Checkbox explicitly disabled by
setting enabled = false to convey it’s a read-only indicator (keep Row.clickable
and onToggle as-is), or move the interaction into the Checkbox by giving it
onCheckedChange = { onToggle(it) } (and remove Row.clickable) so the Checkbox is
the sole interactive element; update the Checkbox instantiation accordingly and
ensure onToggle and selected are used consistently.
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanReschedules/LoanReschedulesViewModel.kt`:
- Around line 51-58: When handling LoanReschedulesAction.OnDeleteIconClick and
OnApproveIconClick in LoanReschedulesViewModel, and before performing
ConfirmDelete/ConfirmApprove flows (and the similar handlers around the 85-127
region), first verify the target item's status is "Pending" on the current state
instead of dispatching by rescheduleId unconditionally; only call
mutableStateFlow.update to set dialogState =
LoanReschedulesUiState.DialogState.ConfirmDelete(action.item.id) or
ConfirmApprove(action.item.id) if the looked-up item's status == Pending, and
likewise check the item's status inside the ConfirmDelete ->
deleteReschedule(rescheduleId) and ConfirmApprove ->
approveReschedule(rescheduleId) branches to abort if not Pending. Ensure you
reference the existing mutableStateFlow, LoanReschedulesUiState.DialogState, and
the deleteReschedule/approveReschedule methods when adding these guards so the
logic is consistently enforced.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 70cfbdcb-22ef-46a1-b118-2bce11f8e46e
📒 Files selected for processing (26)
core/data/src/commonMain/kotlin/com/mifos/core/data/di/RepositoryModule.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repository/LoanReschedulesRepository.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/LoanReschedulesRepositoryImpl.ktcore/database/src/commonMain/kotlin/com/mifos/room/basemodel/APIEndPoint.ktcore/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/theme/DesignToken.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/reschedules/LoanRescheduleApprovalRequest.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/reschedules/LoanRescheduleRejectionRequest.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/reschedules/LoanRescheduleRequest.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/reschedules/LoanRescheduleResponse.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/reschedules/LoanRescheduleTemplate.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/reschedules/RescheduleReasonOption.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/reschedules/RescheduleStatus.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/reschedules/RescheduleTimeline.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerLoan.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/services/LoanService.ktfeature/loan/src/commonMain/composeResources/values/strings.xmlfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/createLoanReschedules/LoanRescheduleFormScreen.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/createLoanReschedules/LoanRescheduleFormViewModel.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/createLoanReschedules/LoanReschedulesFormScreenRoute.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/di/LoanModule.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/LoanAccountProfileNavigation.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/LoanAccountProfileScreen.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanReschedules/LoanReschedulesScreen.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanReschedules/LoanReschedulesScreenRoute.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanReschedules/LoanReschedulesViewModel.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/navigation/LoanNavigation.kt
🚧 Files skipped from review as they are similar to previous changes (6)
- core/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/reschedules/RescheduleTimeline.kt
- feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/LoanAccountProfileNavigation.kt
- core/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/reschedules/RescheduleStatus.kt
- core/database/src/commonMain/kotlin/com/mifos/room/basemodel/APIEndPoint.kt
- core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/theme/DesignToken.kt
- core/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/reschedules/LoanRescheduleRequest.kt
kartikey004
force-pushed
the
feature/loan-reschedules
branch
from
March 15, 2026 13:10
45ef6cb to
0c9a769
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/createLoanReschedules/LoanRescheduleFormViewModel.kt (1)
136-145:⚠️ Potential issue | 🟠 MajorSend
nullfor unset optional fields.Lines 153-186 still serialize disabled/blank optionals as
"", even though the request model now supports omission. Fineract's reschedule docs markrescheduleReasonCommentas optional and show unused optional fields sent asnull, not empty strings, so these placeholders are the wrong shape for date/number inputs and can trigger server-side validation. Reusestate.isSubmitEnabledinvalidateAndSubmit()and switch these branches tonull/takeIf { it.isNotBlank() }. (fineract.apache.org)🔧 Suggested fix
private fun validateAndSubmit() { if (state.selectedReasonId == null) { mutableStateFlow.update { it.copy(reasonIdError = Res.string.feature_loan_must_select_reason) } return } + if (!state.isSubmitEnabled) return mutableStateFlow.update { it.copy(reasonIdError = null) } submitReschedule() } @@ - rescheduleReasonComment = state.comments, - adjustedDueDate = if (state.changeRepaymentDateSelected) { - state.adjustedDueDate - } else { - "" - }, - graceOnPrincipal = if (state.introduceGracePeriodsSelected) { - state.graceOnPrincipal - } else { - "" - }, - graceOnInterest = if (state.introduceGracePeriodsSelected) { - state.graceOnInterest - } else { - "" - }, - extraTerms = if (state.extendRepaymentPeriodSelected) { - state.extraTerms - } else { - "" - }, - newInterestRate = if (state.adjustInterestRateSelected) { - state.newInterestRate - } else { - "" - }, + rescheduleReasonComment = state.comments.takeIf { it.isNotBlank() }, + adjustedDueDate = state.adjustedDueDate.takeIf { + state.changeRepaymentDateSelected && it.isNotBlank() + }, + graceOnPrincipal = state.graceOnPrincipal.takeIf { + state.introduceGracePeriodsSelected && it.isNotBlank() + }, + graceOnInterest = state.graceOnInterest.takeIf { + state.introduceGracePeriodsSelected && it.isNotBlank() + }, + extraTerms = state.extraTerms.takeIf { + state.extendRepaymentPeriodSelected && it.isNotBlank() + }, + newInterestRate = state.newInterestRate.takeIf { + state.adjustInterestRateSelected && it.isNotBlank() + },Also applies to: 153-186
🤖 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/createLoanReschedules/LoanRescheduleFormViewModel.kt` around lines 136 - 145, The form currently serializes unset optional fields as empty strings instead of nulls; update validateAndSubmit() to rely on state.isSubmitEnabled and when building the request map/DTO set optional text fields like rescheduleReasonComment to value.takeIf { it.isNotBlank() } (or null when disabled) and set optional date/number fields to null when not provided, ensuring the request omits/uses null for unused optionals rather than "" to match the Fineract reschedule model.feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/createLoanReschedules/LoanRescheduleFormScreen.kt (1)
306-315:⚠️ Potential issue | 🟠 MajorUse
toggleablehere instead of a clickable disabled checkbox.The row is interactive, but Line 314 renders the checkbox as disabled. That gives the wrong visual state and drops proper checkbox semantics for accessibility. Make the row the checkbox owner via
toggleable(role = Role.Checkbox)and keep the checkbox non-owning.Suggested fix
-import androidx.compose.foundation.clickable +import androidx.compose.foundation.selection.toggleable ... +import androidx.compose.ui.semantics.Role ... Row( verticalAlignment = Alignment.CenterVertically, modifier = Modifier .fillMaxWidth() .clip(KptTheme.shapes.small) - .clickable { onToggle(!selected) } + .toggleable( + value = selected, + role = Role.Checkbox, + onValueChange = onToggle, + ) .padding(vertical = KptTheme.spacing.xs), ) { - Checkbox(checked = selected, onCheckedChange = null, enabled = false) + Checkbox(checked = selected, onCheckedChange = null) Text(🤖 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/createLoanReschedules/LoanRescheduleFormScreen.kt` around lines 306 - 315, Replace the Row's clickable with a toggleable so the Row owns the checkbox semantics: in LoanRescheduleFormScreen change Modifier.clickable { onToggle(!selected) } to Modifier.toggleable(value = selected, onValueChange = { onToggle(it) }, role = Role.Checkbox) and update the Checkbox to be non-owning but visually enabled by using Checkbox(checked = selected, onCheckedChange = null, enabled = true) so the Row handles interaction/accessibility while the Checkbox reflects state.
🧹 Nitpick comments (1)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanReschedules/LoanReschedulesViewModel.kt (1)
52-74: Rename this flow torejectso the API matches the behavior.
ConfirmDelete/deleteReschedule()reads like a destructive remove, but this path only transitions the item toREJECTED. Keeping delete terminology in the ViewModel API makes future maintenance error-prone.Based on learnings, the
deleteReschedulemethod intentionally delegates todataManagerLoan.rejectLoanReschedule(), and the "delete" UI action is designed to mark the request as REJECTED, not physically remove it from the database.Also applies to: 123-143, 150-166
🤖 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/loanReschedules/LoanReschedulesViewModel.kt` around lines 52 - 74, Rename the "delete" API to "reject" so the ViewModel matches the behavior: replace LoanReschedulesAction.ConfirmDelete and LoanReschedulesUiState.DialogState.ConfirmDelete usages with a ConfirmReject/ConfirmReject dialog variant, rename the deleteReschedule(rescheduleId) function to rejectReschedule(rescheduleId) and call it from the ConfirmReject branch, and ensure rejectReschedule delegates to dataManagerLoan.rejectLoanReschedule(). Update all other occurrences (the other blocks referenced) so the confirm+method names consistently use "reject" instead of "delete".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@feature/loan/src/commonMain/composeResources/values/strings.xml`:
- Around line 451-459: Update the UI copy for the "delete" action to use
reject/rejected wording because deleteReschedule delegates to
dataManagerLoan.rejectLoanReschedule() and the action marks the request as
REJECTED rather than removing it; replace the string resources
feature_loan_reschedule_delete_title, feature_loan_reschedule_delete_message,
feature_loan_reschedule_delete_confirm, and
feature_loan_reschedule_delete_failed with wording like "Reject Reschedule",
"Are you sure you want to reject this reschedule? This action will mark the
request as rejected.", "Reject", and "Failed to reject reschedule." so the UX
matches the underlying rejectLoanReschedule behavior.
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/createLoanReschedules/LoanRescheduleFormScreen.kt`:
- Around line 89-92: LoanRescheduleFormScreen places MifosBreadcrumbNavBar and
RescheduleFormContent in a Column without constraints, so
RescheduleFormContent's fillMaxSize() breaks scrolling and hides the bottom of
the form; fix by constraining the form with weight(1f) — e.g. give
RescheduleFormContent a Modifier.weight(1f) (or wrap it in a Box/Column with
Modifier.weight(1f)) so the scroll viewport measures correctly while keeping the
breadcrumb visible.
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanReschedules/LoanReschedulesScreen.kt`:
- Around line 104-123: The outer Column in LoanReschedulesScreen does not
constrain height so RescheduleListContent (which uses fillMaxSize() internally)
overmeasures; update the layout so the Column or RescheduleListContent is given
a bounded height — for example, give the outer Column a Modifier.fillMaxSize()
and/or wrap RescheduleListContent with a sizing modifier that takes remaining
space (e.g., use weight(1f) or fillMaxHeight()) so the MifosBreadcrumbNavBar is
measured first and RescheduleListContent gets the remaining viewport; adjust in
the LoanReschedulesScreen where MifosBreadcrumbNavBar and RescheduleListContent
are composed.
---
Duplicate comments:
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/createLoanReschedules/LoanRescheduleFormScreen.kt`:
- Around line 306-315: Replace the Row's clickable with a toggleable so the Row
owns the checkbox semantics: in LoanRescheduleFormScreen change
Modifier.clickable { onToggle(!selected) } to Modifier.toggleable(value =
selected, onValueChange = { onToggle(it) }, role = Role.Checkbox) and update the
Checkbox to be non-owning but visually enabled by using Checkbox(checked =
selected, onCheckedChange = null, enabled = true) so the Row handles
interaction/accessibility while the Checkbox reflects state.
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/createLoanReschedules/LoanRescheduleFormViewModel.kt`:
- Around line 136-145: The form currently serializes unset optional fields as
empty strings instead of nulls; update validateAndSubmit() to rely on
state.isSubmitEnabled and when building the request map/DTO set optional text
fields like rescheduleReasonComment to value.takeIf { it.isNotBlank() } (or null
when disabled) and set optional date/number fields to null when not provided,
ensuring the request omits/uses null for unused optionals rather than "" to
match the Fineract reschedule model.
---
Nitpick comments:
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanReschedules/LoanReschedulesViewModel.kt`:
- Around line 52-74: Rename the "delete" API to "reject" so the ViewModel
matches the behavior: replace LoanReschedulesAction.ConfirmDelete and
LoanReschedulesUiState.DialogState.ConfirmDelete usages with a
ConfirmReject/ConfirmReject dialog variant, rename the
deleteReschedule(rescheduleId) function to rejectReschedule(rescheduleId) and
call it from the ConfirmReject branch, and ensure rejectReschedule delegates to
dataManagerLoan.rejectLoanReschedule(). Update all other occurrences (the other
blocks referenced) so the confirm+method names consistently use "reject" instead
of "delete".
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b8db0759-847a-4762-a573-d7c4450137c1
📒 Files selected for processing (26)
core/data/src/commonMain/kotlin/com/mifos/core/data/di/RepositoryModule.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repository/LoanReschedulesRepository.ktcore/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/LoanReschedulesRepositoryImpl.ktcore/database/src/commonMain/kotlin/com/mifos/room/basemodel/APIEndPoint.ktcore/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/theme/DesignToken.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/reschedules/LoanRescheduleApprovalRequest.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/reschedules/LoanRescheduleRejectionRequest.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/reschedules/LoanRescheduleRequest.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/reschedules/LoanRescheduleResponse.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/reschedules/LoanRescheduleTemplate.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/reschedules/RescheduleReasonOption.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/reschedules/RescheduleStatus.ktcore/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/reschedules/RescheduleTimeline.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerLoan.ktcore/network/src/commonMain/kotlin/com/mifos/core/network/services/LoanService.ktfeature/loan/src/commonMain/composeResources/values/strings.xmlfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/createLoanReschedules/LoanRescheduleFormScreen.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/createLoanReschedules/LoanRescheduleFormViewModel.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/createLoanReschedules/LoanReschedulesFormScreenRoute.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/di/LoanModule.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/LoanAccountProfileNavigation.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/LoanAccountProfileScreen.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanReschedules/LoanReschedulesScreen.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanReschedules/LoanReschedulesScreenRoute.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanReschedules/LoanReschedulesViewModel.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/navigation/LoanNavigation.kt
🚧 Files skipped from review as they are similar to previous changes (13)
- core/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/reschedules/LoanRescheduleApprovalRequest.kt
- core/database/src/commonMain/kotlin/com/mifos/room/basemodel/APIEndPoint.kt
- core/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/reschedules/RescheduleTimeline.kt
- core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/theme/DesignToken.kt
- core/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/LoanReschedulesRepositoryImpl.kt
- feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/createLoanReschedules/LoanReschedulesFormScreenRoute.kt
- core/data/src/commonMain/kotlin/com/mifos/core/data/repository/LoanReschedulesRepository.kt
- core/network/src/commonMain/kotlin/com/mifos/core/network/services/LoanService.kt
- feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountProfile/LoanAccountProfileNavigation.kt
- feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/navigation/LoanNavigation.kt
- core/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/reschedules/RescheduleStatus.kt
- core/model/src/commonMain/kotlin/com/mifos/core/model/objects/account/loan/reschedules/LoanRescheduleTemplate.kt
- feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/di/LoanModule.kt
kartikey004
force-pushed
the
feature/loan-reschedules
branch
from
March 15, 2026 16:18
0c9a769 to
557a704
Compare
itsPronay
suggested changes
Mar 16, 2026
Comment on lines
+91
to
+96
| Column(modifier = Modifier.fillMaxSize()) { | ||
| MifosBreadcrumbNavBar(navController = navController) | ||
| Box(modifier = Modifier.weight(1f)) { | ||
| RescheduleFormContent(state = state, onAction = viewModel::trySendAction) | ||
| } | ||
| } |
Member
There was a problem hiding this comment.
Please create a new composable called RescheduleFormScreen or RescheduleFormContent and put it there.
RescheduleFormScreen sounds more reasonable tho
| onDateSelected: (String) -> Unit, | ||
| modifier: Modifier = Modifier, | ||
| ) { | ||
| var showDialog by remember { mutableStateOf(false) } |
Member
There was a problem hiding this comment.
Should be inside LoanRescheduleUiState
Comment on lines
+98
to
+102
| LaunchedEffect(lifecycleOwner) { | ||
| lifecycleOwner.repeatOnLifecycle(Lifecycle.State.RESUMED) { | ||
| viewModel.trySendAction(LoanReschedulesAction.RefreshList) | ||
| } | ||
| } |
Member
There was a problem hiding this comment.
why is it needed? We have similar code inside init in the viewModel
Contributor
Author
There was a problem hiding this comment.
init block handles only the initial load, but this LaunchedEffect on the RESUMED state is to handle back navigation from LoanReschedulesFormScreen after the successful submission of a new request. in this case the init of the ViewModel will not run again.
Comment on lines
+121
to
+133
| internal fun RescheduleListContent( | ||
| state: LoanReschedulesUiState, | ||
| onAddClick: () -> Unit, | ||
| onRetry: () -> Unit, | ||
| onDeleteClick: (LoanRescheduleResponse) -> Unit, | ||
| onApproveClick: (LoanRescheduleResponse) -> Unit, | ||
| onAction: (LoanReschedulesAction) -> Unit, | ||
| ) { | ||
| Box(modifier = Modifier.fillMaxSize()) { | ||
| Column( | ||
| modifier = Modifier | ||
| .fillMaxSize(), | ||
| ) { |
Member
There was a problem hiding this comment.
is RescheduleListContent scrollable? Can the user scroll this page when they are in horizontal mode!
same for the other screen
Contributor
Author
There was a problem hiding this comment.
RescheduleFormContent is scrollable. in RescheduleListContent the outer screen is static so the title stays at the top, but the inner table has .verticalScroll(rememberScrollState()) so the data is fully scrollable.
Comment on lines
+104
to
+118
| Column(modifier = Modifier.fillMaxSize()) { | ||
| MifosBreadcrumbNavBar(navController = navController) | ||
|
|
||
| Box(modifier = Modifier.weight(1f)) { | ||
| RescheduleListContent( | ||
| state = state, | ||
| onAddClick = { navController.navigateToLoanRescheduleFormScreen(viewModel.loanId) }, | ||
| onRetry = { viewModel.trySendAction(LoanReschedulesAction.OnRetryFetching) }, | ||
| onDeleteClick = { item -> viewModel.trySendAction(LoanReschedulesAction.OnDeleteIconClick(item)) }, | ||
| onApproveClick = { item -> viewModel.trySendAction(LoanReschedulesAction.OnApproveIconClick(item)) }, | ||
| onAction = viewModel::trySendAction, | ||
| ) | ||
| } | ||
| } | ||
| } |
Member
There was a problem hiding this comment.
I would suggest creating a composable called RescheduleListScreen and put it there. It breaks consistency
| ) | ||
| } | ||
|
|
||
| when (val dialog = state.dialogState) { |
Member
There was a problem hiding this comment.
There should be a separate composable for dialogStates
Comment on lines
+318
to
+340
| is LoanReschedulesUiState.DialogState.ConfirmApprove -> { | ||
| MifosDialogBox( | ||
| showDialogState = true, | ||
| title = stringResource(Res.string.feature_loan_reschedule_approve_title), | ||
| message = stringResource(Res.string.feature_loan_reschedule_approve_message), | ||
| confirmButtonText = stringResource(Res.string.feature_loan_reschedule_approve_confirm), | ||
| dismissButtonText = stringResource(Res.string.feature_loan_reschedule_cancel), | ||
| onConfirm = { onAction(LoanReschedulesAction.ConfirmApprove(dialog.rescheduleId)) }, | ||
| onDismiss = { onAction(LoanReschedulesAction.DismissDialog) }, | ||
| ) | ||
| } | ||
|
|
||
| is LoanReschedulesUiState.DialogState.ActionError -> { | ||
| MifosDialogBox( | ||
| showDialogState = true, | ||
| title = stringResource(Res.string.feature_loan_reschedule_failure_title), | ||
| message = stringResource(dialog.messageRes), | ||
| confirmButtonText = stringResource(Res.string.feature_loan_reschedule_ok), | ||
| dismissButtonText = "", | ||
| onConfirm = { onAction(LoanReschedulesAction.DismissDialog) }, | ||
| onDismiss = { onAction(LoanReschedulesAction.DismissDialog) }, | ||
| ) | ||
| } |
Member
There was a problem hiding this comment.
Since this is a status dialog, and it has 2 states, either
- Success
- Failure
so you should use MifosStatusDialog here
kartikey004
force-pushed
the
feature/loan-reschedules
branch
from
March 17, 2026 15:17
0512fbd to
d0a2961
Compare
itsPronay
suggested changes
Mar 18, 2026
Comment on lines
+399
to
+407
| val today = ApiDateFormatter.formatForApi(Clock.System.now().toEpochMilliseconds()) | ||
| val response = mBaseApiManager.loanService.approveLoanReschedule( | ||
| scheduleId = scheduleId, | ||
| request = LoanRescheduleApprovalRequest( | ||
| approvedOnDate = today, | ||
| dateFormat = ApiDateFormatter.DATE_FORMAT, | ||
| locale = ApiDateFormatter.LOCALE, | ||
| ), | ||
| ) |
Member
There was a problem hiding this comment.
You should do it in the view model, build LoanRescheduleApprovalRequest in viewmodel, and pass it on
suspend fun approveLoanReschedule(scheduleId: Int, req : LoanRescheduleApprovalRequest)
Comment on lines
+414
to
+421
| val response = mBaseApiManager.loanService.rejectLoanReschedule( | ||
| scheduleId = scheduleId, | ||
| request = LoanRescheduleRejectionRequest( | ||
| rejectedOnDate = today, | ||
| dateFormat = ApiDateFormatter.DATE_FORMAT, | ||
| locale = ApiDateFormatter.LOCALE, | ||
| ), | ||
| ) |
Member
There was a problem hiding this comment.
same here, build LoanRescheduleRejectionRequest in the view model and pass it here
niyajali
previously requested changes
Mar 18, 2026
| dataManagerLoan.submitLoanReschedule(request) | ||
| DataState.Success(Unit) | ||
| } catch (e: CancellationException) { | ||
| throw e |
Collaborator
There was a problem hiding this comment.
we won't need to catch CancellationException, wrap all the methods with withContext(ioDispatcher), use the DispatcherManager from core-base/common module to use the ioDispatcher.
|
|
||
| override fun getLoanReschedules(loanId: Int): Flow<DataState<List<LoanRescheduleResponse>>> { | ||
| return dataManagerLoan.getLoanReschedules(loanId) | ||
| .asDataStateFlow() |
Collaborator
There was a problem hiding this comment.
Use flowOn(ioDispatcher) operator on all non-suspend flowable methods..
itsPronay
reviewed
Mar 20, 2026
Comment on lines
+168
to
+172
| adjustedDueDate = if (state.changeRepaymentDateSelected) state.adjustedDueDate else "", | ||
| graceOnPrincipal = if (state.introduceGracePeriodsSelected) state.graceOnPrincipal else "", | ||
| graceOnInterest = if (state.introduceGracePeriodsSelected) state.graceOnInterest else "", | ||
| extraTerms = if (state.extendRepaymentPeriodSelected) state.extraTerms else "", | ||
| newInterestRate = if (state.adjustInterestRateSelected) state.newInterestRate else "", |
Member
There was a problem hiding this comment.
Are we supposed to send empty values with the request?
Contributor
Author
There was a problem hiding this comment.
@itsPronay yes. i checked with the web app.
itsPronay
approved these changes
Mar 21, 2026
biplab1
approved these changes
Mar 25, 2026
biplab1
left a comment
biplab1
left a comment
Contributor
There was a problem hiding this comment.
Looks good to me. This can be merged.
|
This was referenced Apr 24, 2026
Closed
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
4 participants
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.
Fixes - Jira-#670
Description - Implemented the Loan Reschedules feature to view, create, approve and delete reschedule requests.
Video:
Screen_recording_20260315_144311.webm
Summary by CodeRabbit
New Features
UI / Localization