Approve button in recurring account Issue#2580
Approve button in recurring account Issue#2580ufuomaisaac wants to merge 37 commits intoopenMF:developmentfrom
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 recurring deposit approval feature: new approval model, API/service/datamanager/repository + use-case and ViewModel, navigation and approval UI (with date picker and lifecycle refresh), DateHelper date-format helper, DI wiring, strings, and a Ktor JSON config tweak. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(220,230,241,0.5)
participant User
end
rect rgba(200,240,200,0.5)
participant Screen
end
rect rgba(255,230,200,0.5)
participant VM
end
rect rgba(240,200,255,0.5)
participant UseCase
end
rect rgba(255,220,220,0.5)
participant Repo
end
rect rgba(220,255,240,0.5)
participant Service
end
rect rgba(240,240,220,0.5)
participant API
end
User->>Screen: pick date, enter reason, tap Save
Screen->>VM: approveRecurringDepositApplication(accountId, approval)
VM->>VM: emit ShowProgressbar
VM->>UseCase: invoke(accountId, approval)
UseCase->>Repo: approveRecurringDepositAccount(accountId, approval)
Repo->>Service: approveRecurringDepositAccount(accountId, approval)
Service->>API: POST /recurringdepositaccounts/{accountId}?command=approve
API-->>Service: HttpResponse
Service-->>Repo: HttpResponse
Repo->>Repo: decode -> GenericResponse or throw
Repo-->>UseCase: DataState.Success / DataState.Error
UseCase-->>VM: DataState
VM-->>Screen: update UI state (success / error)
Screen->>User: show snackbar/dialog and navigate back on success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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: 8
🤖 Fix all issues with AI agents
In `@core/common/src/commonMain/kotlin/com/mifos/core/common/utils/DateHelper.kt`:
- Around line 44-50: The apiDateFormat constant currently uses day() which
produces non-padded days; replace that call with dayOfMonth(Padding.ZERO) in the
apiDateFormat formatter (the LocalDateTime.Format block) so dates are rendered
as "dd MMMM yyyy" (e.g., "05 January 2026") to match the API and the other date
format constants referenced by RecurringDepositAccountApprovalScreen.kt:223.
In
`@core/network/src/androidMain/kotlin/com/mifos/core/network/KtorHttpClient.android.kt`:
- Around line 31-41: The file currently installs ContentNegotiation twice which
causes DuplicatePluginException; remove the redundant
install(ContentNegotiation) block and keep a single installation that preserves
the desired Json settings (ensure ignoreUnknownKeys = true, encodeDefaults =
true, isLenient = true, prettyPrint = true, explicitNulls = false) so the final
install(ContentNegotiation) { json(Json { ... }) } contains all required
options; verify only one call to install(ContentNegotiation) exists and run a
quick build to confirm HttpClient initializes without the duplicate plugin
error.
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/navigation/ClientNavigation.kt`:
- Around line 254-265: clientRecurringDepositAccountDestination is passing
onViewAccount as an empty lambda and onApproveAccount forwards accountNumber to
navigateToRecurringDepositAccountApproval which expects accountId; wire the
onViewAccount callback to navigate to the appropriate account view (e.g., call
navController.navigateToRecurringDepositAccount with the same identifier) and
make identifier names consistent: either rename accountNumber to accountId in
the onApproveAccount lambda or adapt the
navigateToRecurringDepositAccountApproval invocation to accept accountNumber,
ensuring RecurringDepositAccountScreen's OnViewAccount event and
navigateToRecurringDepositAccountApproval use the same identifier name/type.
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/recurringDepositAccount/RecurringDepositAccountScreen.kt`:
- Line 178: Fix the typo in the TODO comment inside
RecurringDepositAccountScreen (RecurringDepositAccountScreen.kt): replace
"clientimplement" with either "client implement" or simply "implement" (e.g.,
"todo implement search bar functionality") so the TODO reads clearly and
consistently with other comments.
- Around line 76-78: The LaunchedEffect keyed on
navController.currentBackStackEntry causes a duplicate fetch because the
ViewModel already loads data in its init via
checkNetworkAndRecurringDepositAccounts(); remove or change this LaunchedEffect
to avoid the immediate re-fetch: either delete the LaunchedEffect that calls
viewModel.trySendAction(RecurringDepositAccountAction.Refresh) or replace it
with a targeted mechanism that only triggers on return (e.g., observe
NavBackStackEntry.lifecycle events or a navigation result/SharedViewModel flag)
so that refresh runs only when coming back from another screen rather than on
initial composition.
In
`@feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/recurringAccountApproval/RecurringDepositAccountApprovalScreen.kt`:
- Around line 113-121: Replace the hardcoded dialogTitle "OK" in the
RecurringDepositAccountApprovalUiState.ShowRecurringDepositAccountApprovedSuccessfully
branch with a localized string resource; update the MifosAlertDialog call to use
stringResource(Res.string.xxx) (e.g., Res.string.ok or a feature-specific
resource) for dialogTitle so the title is internationalized while keeping
onConfirmation calling navigateBack and other parameters unchanged.
- Line 235: The file defines a local no-op function rememberDatePickerState that
shadows the imported API and causes datePickerState to be Unit; remove this
stubbed function (the rememberDatePickerState declaration) so the code uses the
correct imported rememberDatePickerState and datePickerState will have the
proper type to access selectedDateMillis, or if you intentionally need a local
helper, rename it and match the real API signature to return the proper state
instead of Unit.
- Around line 142-147: The date picker currently uses
SelectableDates.isSelectableDate to allow only dates >= Clock.System.now(),
blocking today and past dates; update the logic in the
datePickerState/selectableDates block (SelectableDates.isSelectableDate) to
allow today and earlier by inverting the comparison (use utcTimeMillis <=
Clock.System.now().toEpochMilliseconds()) or, better, compare dates by LocalDate
(convert utcTimeMillis and now to LocalDate and allow selectable when
candidateDate <= today) so same-day approvals are permitted; adjust the
implementation in RecurringDepositAccountApprovalScreen where datePickerState is
created.
🧹 Nitpick comments (5)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/recurringDepositAccount/RecurringDepositAccountScreen.kt (1)
232-250: Empty string fallback for nullaccountNomay cause silent failures.Passing an empty string when
accountNois null (lines 237, 244) could lead to confusing behavior or failed API calls downstream. Consider either:
- Filtering out items with null
accountNofrom displaying actions, or- Handling the null case explicitly (e.g., showing an error dialog)
Suggested approach: guard against null before dispatching action
onActionClicked = { actions -> + val accountNo = recurringDeposit.accountNo + if (accountNo.isNullOrEmpty()) return@MifosActionsSavingsListingComponent when (actions) { is Actions.ViewAccount -> { onAction( RecurringDepositAccountAction.ViewAccount( - recurringDeposit.accountNo ?: "", + accountNo, ) ) } is Actions.ApproveAccount -> { onAction( RecurringDepositAccountAction.ApproveAccount( - recurringDeposit.accountNo ?: "", + accountNo, ) ) } else -> Unit } }feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/di/RecurringDepositModule.kt (1)
22-33: Consider usingviewModelOffor consistency.The existing
RecurringAccountViewModelusesviewModelOf(::RecurringAccountViewModel)(line 20), while the newRecurringDepositAccountApprovalViewModeluses explicit parameter injection. IfSavedStateHandleandApproveRecurringDepositUseCaseare both available via DI, you could simplify:♻️ Suggested simplification
- viewModel { - RecurringDepositAccountApprovalViewModel( - savedStateHandle = get(), - approveRecurringDepositUseCase = get() - ) - } + viewModelOf(::RecurringDepositAccountApprovalViewModel) - factory { - ApproveRecurringDepositUseCase( - repository = get() - ) - } + factory(::ApproveRecurringDepositUseCase)core/network/src/commonMain/kotlin/com/mifos/core/network/services/RecurringAccountService.kt (1)
37-41: UseAPIEndPoint.CREATE_RECURRING_DEPOSIT_ACCOUNTSfor consistency.Lines 26 and 31 use
APIEndPoint.CREATE_RECURRING_DEPOSIT_ACCOUNTS, but this endpoint uses a hardcoded string. For maintainability and consistency with the rest of the interface, apply the suggested refactor:♻️ Suggested refactor
- `@POST`("recurringdepositaccounts/{accountId}?command=approve") + `@POST`(APIEndPoint.CREATE_RECURRING_DEPOSIT_ACCOUNTS + "/{accountId}?command=approve") fun approveRecurringDepositAccount( `@Path`("accountId") accountId: String, `@Body` approval: RecurringDepositApproval, ): Flow<HttpResponse>core/model/src/commonMain/kotlin/com/mifos/core/model/objects/template/recurring/approval/RecurringDepositApproval.kt (1)
15-22: Prefer immutable properties for request payloads.
These fields look write-once; switching tovalavoids accidental mutation.♻️ Suggested change
-data class RecurringDepositApproval( - var locale: String = "en", - var dateFormat: String = "dd MMMM yyyy", - var approvedOnDate: String? = null, - var note: String? = null, -) +data class RecurringDepositApproval( + val locale: String = "en", + val dateFormat: String = "dd MMMM yyyy", + val approvedOnDate: String? = null, + val note: String? = null, +)core/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerRecurringAccount.kt (1)
56-71: Consider extracting the shared response-mapping logic.
This duplicates the error/success parsing increateRecurringDepositAccount; a helper keeps it consistent and easier to maintain.
core/common/src/commonMain/kotlin/com/mifos/core/common/utils/DateHelper.kt
Show resolved
Hide resolved
| install(ContentNegotiation) { | ||
| json( | ||
| Json { | ||
| ignoreUnknownKeys = true | ||
| encodeDefaults = true | ||
| isLenient = true | ||
| prettyPrint = true | ||
| explicitNulls = false | ||
| }, | ||
| ) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f "KtorHttpClient.android.kt" | head -5Repository: openMF/android-client
Length of output: 243
🏁 Script executed:
cat -n core/network/src/androidMain/kotlin/com/mifos/core/network/KtorHttpClient.android.kt | head -80Repository: openMF/android-client
Length of output: 2500
🌐 Web query:
Ktor HttpClient ContentNegotiation install twice duplicate plugin exception
💡 Result:
Cause
- The exception means ContentNegotiation was installed more than once with the same plugin key (e.g., installed twice on the same HttpClient or both globally and again in a test/server scope). Ktor throws DuplicatePluginException when a plugin is installed twice. [1][2]
How to fix
- Ensure you install ContentNegotiation only once per client (or per application scope). Remove the duplicate install (common places: shared client factory, tests that call module twice, or installing on both a parent route and its child incorrectly). [1][3]
How to check/guard in code (client)
- Use pluginOrNull to see if the plugin is already installed before installing:
- if (client.pluginOrNull(ContentNegotiation) == null) { /* install */ }
- or when creating clients, centralize installation in one factory so callers reuse the same configured client. [2]
Example (HttpClient factory)
- val client = HttpClient(CIO) {
install(ContentNegotiation) { json() }
}
Testing tip
- If tests start your app twice (EngineMain vs application { module(...) } or you call module twice), remove the duplicate startup or use embeddedServer only in one place to avoid reinstalling server plugins. StackOverflow examples show this pattern as a common cause. [3]
Sources
- Ktor Client ContentNegotiation docs. [1]
- Ktor API: pluginOrNull for HttpClient. [2]
- StackOverflow discussion about DuplicatePluginException when modules/tests start things twice. [3]
Consolidate duplicate ContentNegotiation installations (causes DuplicatePluginException).
This file installs ContentNegotiation twice (lines 31–41 and lines 53–62). Ktor throws DuplicatePluginException when the same plugin is installed more than once, preventing HttpClient initialization. Keep one install(ContentNegotiation) block with your desired JSON configuration. Note: the first block includes encodeDefaults = true, which the second lacks—ensure you retain the options you need.
🤖 Prompt for AI Agents
In
`@core/network/src/androidMain/kotlin/com/mifos/core/network/KtorHttpClient.android.kt`
around lines 31 - 41, The file currently installs ContentNegotiation twice which
causes DuplicatePluginException; remove the redundant
install(ContentNegotiation) block and keep a single installation that preserves
the desired Json settings (ensure ignoreUnknownKeys = true, encodeDefaults =
true, isLenient = true, prettyPrint = true, explicitNulls = false) so the final
install(ContentNegotiation) { json(Json { ... }) } contains all required
options; verify only one call to install(ContentNegotiation) exists and run a
quick build to confirm HttpClient initializes without the duplicate plugin
error.
| clientRecurringDepositAccountDestination( | ||
| navController = navController, | ||
| navigateBack = navController::popBackStack, | ||
| {}, | ||
| {}, | ||
| onApproveAccount = { accountNumber -> | ||
| navController.navigateToRecurringDepositAccountApproval(accountNumber) | ||
| }, | ||
| onViewAccount = {}, | ||
| ) | ||
|
|
||
| recurringDepositAccountApprovalDestination( | ||
| navigateBack = navController::popBackStack, | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for clientRecurringDepositAccountDestination function definition
rg -n "fun.*clientRecurringDepositAccountDestination" --type=kt -A 10Repository: openMF/android-client
Length of output: 186
🏁 Script executed:
#!/bin/bash
# Search for recurringDepositAccountApprovalDestination and how it uses parameters
rg -n "fun.*recurringDepositAccountApprovalDestination" --type=kt -A 10Repository: openMF/android-client
Length of output: 186
🏁 Script executed:
#!/bin/bash
# Search for how onApproveAccount callback is defined in the recurring deposit screen
rg -n "onApproveAccount" --type=kt -B 3 -A 3Repository: openMF/android-client
Length of output: 91
🏁 Script executed:
#!/bin/bash
# Search for how onViewAccount callback is defined/used
rg -n "onViewAccount" --type=kt -B 3 -A 3Repository: openMF/android-client
Length of output: 91
🏁 Script executed:
#!/bin/bash
# Check the navigateToRecurringDepositAccountApproval extension function
rg -n "navigateToRecurringDepositAccountApproval" --type=kt -A 5Repository: openMF/android-client
Length of output: 91
🏁 Script executed:
#!/bin/bash
# Search for clientRecurringDepositAccountDestination function definition
rg -n "fun.*clientRecurringDepositAccountDestination" -A 10Repository: openMF/android-client
Length of output: 1805
🏁 Script executed:
#!/bin/bash
# Search for recurringDepositAccountApprovalDestination definition
rg -n "fun.*recurringDepositAccountApprovalDestination" -A 10Repository: openMF/android-client
Length of output: 2090
🏁 Script executed:
#!/bin/bash
# Search for onApproveAccount usage patterns
rg -n "onApproveAccount" -B 2 -A 2Repository: openMF/android-client
Length of output: 7632
🏁 Script executed:
#!/bin/bash
# Search for onViewAccount usage patterns
rg -n "onViewAccount" -B 2 -A 2Repository: openMF/android-client
Length of output: 7593
🏁 Script executed:
#!/bin/bash
# Search for navigateToRecurringDepositAccountApproval extension
rg -n "navigateToRecurringDepositAccountApproval" -A 5Repository: openMF/android-client
Length of output: 2888
Wire the onViewAccount callback and verify the account identifier naming.
The onViewAccount callback is actively used by RecurringDepositAccountScreen (triggered on OnViewAccount event), but it's passed as an empty lambda in the navigation setup. Additionally, the onApproveAccount callback passes accountNumber to navigateToRecurringDepositAccountApproval(), which expects a parameter named accountId—verify these represent the same identifier and use consistent naming throughout.
🤖 Prompt for AI Agents
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/navigation/ClientNavigation.kt`
around lines 254 - 265, clientRecurringDepositAccountDestination is passing
onViewAccount as an empty lambda and onApproveAccount forwards accountNumber to
navigateToRecurringDepositAccountApproval which expects accountId; wire the
onViewAccount callback to navigate to the appropriate account view (e.g., call
navController.navigateToRecurringDepositAccount with the same identifier) and
make identifier names consistent: either rename accountNumber to accountId in
the onApproveAccount lambda or adapt the
navigateToRecurringDepositAccountApproval invocation to accept accountNumber,
ensuring RecurringDepositAccountScreen's OnViewAccount event and
navigateToRecurringDepositAccountApproval use the same identifier name/type.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
...ain/kotlin/com/mifos/feature/client/recurringDepositAccount/RecurringDepositAccountScreen.kt
Outdated
Show resolved
Hide resolved
...ain/kotlin/com/mifos/feature/client/recurringDepositAccount/RecurringDepositAccountScreen.kt
Outdated
Show resolved
Hide resolved
...s/feature/recurringDeposit/recurringAccountApproval/RecurringDepositAccountApprovalScreen.kt
Outdated
Show resolved
Hide resolved
...s/feature/recurringDeposit/recurringAccountApproval/RecurringDepositAccountApprovalScreen.kt
Show resolved
Hide resolved
...s/feature/recurringDeposit/recurringAccountApproval/RecurringDepositAccountApprovalScreen.kt
Outdated
Show resolved
Hide resolved
|
@ufuomaisaac Please address the @coderabbitai reviews which are relevant to this PR. Make sure the UI guidelines are being followed as well. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 11400
Critical Issues (Must Fix) 🔴
Important Fixes 🟡
Code Quality Improvements (Nice to Have) 💡
I'm happy to help clarify any of these items or provide more specific guidance on how to fix them. Just let me know! 🤖 |
Alright, will work on that. Thank you! |
|
Tip For best results, initiate chat on the files or code changes.
|
1 similar comment
|
Tip For best results, initiate chat on the files or code changes.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/recurringDepositAccount/RecurringDepositAccountScreen.kt`:
- Around line 232-249: The code currently passes recurringDeposit.accountNo ?:
"" into RecurringDepositAccountAction.ViewAccount and ApproveAccount; instead of
supplying an empty string, first guard for null by reading val accountNo =
recurringDeposit.accountNo and if accountNo is null skip the action (return from
the onActionClicked branch) or surface an error (call the existing error action
handler if available), otherwise call
onAction(RecurringDepositAccountAction.ViewAccount(accountNo)) /
onAction(RecurringDepositAccountAction.ApproveAccount(accountNo)). Update the
onActionClicked lambda handling for Actions.ViewAccount and
Actions.ApproveAccount to use this null check and avoid passing empty
identifiers.
...ain/kotlin/com/mifos/feature/client/recurringDepositAccount/RecurringDepositAccountScreen.kt
Outdated
Show resolved
Hide resolved
|
@biplab1, I have resolved all the issues raised by coderabbitai |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/recurringDepositAccount/RecurringDepositAccountScreen.kt`:
- Around line 78-93: The lifecycle observer currently captures a stale snapshot
of state.recurringDepositAccounts because DisposableEffect is keyed only on
lifecycleOwner; update the observer to read the latest value by using
rememberUpdatedState for the relevant state (e.g., val currentAccounts by
rememberUpdatedState(state.recurringDepositAccounts)) and reference
currentAccounts inside the LifecycleEventObserver callback, keeping the
DisposableEffect keyed on lifecycleOwner and still adding/removing the observer;
ensure viewModel.trySendAction(RecurringDepositAccountAction.Refresh) is invoked
when event == Lifecycle.Event.ON_RESUME and currentAccounts.isNotEmpty().
🧹 Nitpick comments (1)
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/recurringAccountApproval/RecurringDepositAccountApprovalScreen.kt (1)
237-253: Consider narrowing UI-state visibility.These are internal UI details; marking them
internalavoids leaking API surface outside the feature module.♻️ Suggested change
-sealed class RecurringDepositAccountApprovalUiState { +internal sealed class RecurringDepositAccountApprovalUiState { @@ -class RecurringDepositAccountApprovalScreenPreviewProvider : +internal class RecurringDepositAccountApprovalScreenPreviewProvider :
...ain/kotlin/com/mifos/feature/client/recurringDepositAccount/RecurringDepositAccountScreen.kt
Show resolved
Hide resolved
|
@biplab1 I have resolved all the issues that were raised by CodeRabbitAi |
|
@ufuomaisaac, please resolve conflicts and request the team for review. |
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/client/src/commonMain/kotlin/com/mifos/feature/client/recurringDepositAccount/RecurringDepositAccountScreen.kt (1)
304-320:⚠️ Potential issue | 🟡 MinorAdd content descriptions for accessibility.
The search and filter icons have
contentDescription = null, which makes them inaccessible to screen readers. Provide meaningful descriptions for users relying on assistive technologies.♿ Suggested fix
Icon( painter = painterResource(Res.drawable.search), - contentDescription = null, + contentDescription = stringResource(Res.string.search), modifier = Modifier.clickable { onToggleSearch.invoke() }, ) Spacer(modifier = Modifier.width(DesignToken.spacing.largeIncreased)) Icon( painter = painterResource(Res.drawable.filter), - contentDescription = null, + contentDescription = stringResource(Res.string.filter), modifier = Modifier.clickable { onToggleFilter.invoke() }, )
🤖 Fix all issues with AI agents
In
`@feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/recurringAccountApproval/RecurringDepositAccountApprovalScreen.kt`:
- Around line 255-268: The save Button currently allows multiple taps because
there's no loading guard; update the RecurringDepositAccountApprovalContent
signature to accept an isLoading: Boolean = false and inside that composable
disable the Button (use enabled = !isLoading or similar) and prevent calling
approveAccount when isLoading is true; then, where you handle
RecurringDepositAccountApprovalUiState switch, pass isLoading = true when state
== RecurringDepositAccountApprovalUiState.ShowProgressbar and use the
default/false for Initial so the Button is re-enabled; reference symbols:
approveAccount, RecurringDepositApproval,
RecurringDepositAccountApprovalContent, and
RecurringDepositAccountApprovalUiState.ShowProgressbar.
- Around line 260-267: The RecurringDepositApproval payload is using a hardcoded
locale "en" in RecurringDepositAccountApprovalScreen where
approveAccount.invoke(...) is called; replace that literal with the device
default language (e.g., Locale.getDefault().language) in the
RecurringDepositApproval constructor (keep dateFormat and approvedOnDate as-is
using DateHelper.getDateAsStringForApproval(approvalDate) and note =
reasonForApproval). If Locale.getDefault() is not available in commonMain,
implement an expect/actual platform accessor (e.g., getDeviceLanguage()) and use
that instead; apply the same replacement pattern to other payload constructors
across the codebase where "en" is hardcoded.
🧹 Nitpick comments (6)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/recurringDepositAccount/RecurringDepositAccountScreen.kt (3)
128-145: Use string resource for "Error" title and simplify the else branch.The hardcoded
"Error"string should use a string resource for localization consistency. Additionally,else -> nullis unnecessary ifDialogStateis a sealed class with onlyErrorandNonestates—consider removing it or usingelse -> Unit.♻️ Suggested fix
AlertDialog( - title = { Text("Error") }, + title = { Text(stringResource(Res.string.feature_client_dialog_error_title)) }, text = { Text(text = state.dialogState.message) }, ... ) } - else -> null + else -> Unit }You'll need to add a string resource entry for the error title (e.g.,
feature_client_dialog_error_title).
216-222: Preferletblock over non-null assertion for cleaner null handling.While the
!!is safe here due to the preceding null check, using aletblock or smart cast is more idiomatic and avoids the non-null assertion operator.♻️ Suggested fix
lastActive = if (recurringDeposit.status?.submittedAndPendingApproval == true) { stringResource(Res.string.client_savings_pending_approval) - } else if (recurringDeposit.lastActiveTransactionDate != null) { - DateHelper.getDateAsString(recurringDeposit.lastActiveTransactionDate!!) - } else { - notAvailableText - }, + } else { + recurringDeposit.lastActiveTransactionDate?.let { + DateHelper.getDateAsString(it) + } ?: notAvailableText + },
296-299: Use parameterized string resource for better localization.String concatenation
totalItem + " " + stringResource(...)doesn't handle languages with different word orders. Use a parameterized string resource for proper i18n support.🌐 Suggested approach
Define a string resource with a placeholder:
<string name="client_savings_item_count">%1$s items</string>Then use:
Text( text = stringResource(Res.string.client_savings_item_count, totalItem), style = MifosTypography.labelMedium, )feature/client/src/commonMain/kotlin/com/mifos/feature/client/navigation/ClientNavigation.kt (1)
275-280: Empty callbacks for fixed deposit account actions.Both
onApproveAccountandonViewAccountare passed as empty lambdas for the fixed deposit account destination. While this is consistent with the PR's focus on recurring deposits, if the UI exposes approve/view buttons that invoke these callbacks, users will experience no response.Consider either:
- Wiring these callbacks when the corresponding screens are available, or
- Adding TODO comments to track the pending implementation.
feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/recurringAccountApproval/RecurringDepositAccountApprovalScreen.kt (2)
99-101: UnusedSnackbarHostState.The snackbar host state is created and passed to
MifosScaffoldbut never used to show messages. Error states useMifosSweetErrorinstead. Consider removing this if snackbar functionality isn't planned, or use it for transient feedback.
279-279: UnusedGenericResponsein success state.The
responseparameter is stored but never read in the UI—the success dialog shows static localized strings. If the response data isn't needed, this could be simplified to adata object.♻️ Suggested simplification
- data class ShowRecurringDepositAccountApprovedSuccessfully(val response: GenericResponse) : RecurringDepositAccountApprovalUiState() + data object ShowRecurringDepositAccountApprovedSuccessfully : RecurringDepositAccountApprovalUiState()If you plan to display response details (e.g., transaction ID) in the future, keep the current structure.
...s/feature/recurringDeposit/recurringAccountApproval/RecurringDepositAccountApprovalScreen.kt
Show resolved
Hide resolved
| approveAccount.invoke( | ||
| RecurringDepositApproval( | ||
| locale = "en", | ||
| dateFormat = "dd MMMM yyyy", | ||
| approvedOnDate = DateHelper.getDateAsStringForApproval(approvalDate), | ||
| note = reasonForApproval, | ||
| ), | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how locale is handled elsewhere in the codebase
rg -n "locale\s*=\s*\"en\"" --type kotlin -C 2 | head -100Repository: openMF/android-client
Length of output: 13098
🏁 Script executed:
#!/bin/bash
# Search for locale-related code patterns and utilities
rg -n "Locale\." --type kotlin | head -50Repository: openMF/android-client
Length of output: 3017
🏁 Script executed:
#!/bin/bash
# Check if there's a locale utility or provider in the project
fd -t f -n "Locale\|locale" --type f | grep -E "\.kt$" | head -30Repository: openMF/android-client
Length of output: 295
🏁 Script executed:
#!/bin/bash
# Look at the file structure to understand Kotlin Multiplatform setup
ls -la feature/recurringDeposit/src/ 2>/dev/null | head -20Repository: openMF/android-client
Length of output: 232
🏁 Script executed:
#!/bin/bash
# Check the SettingsScreen locale comment
sed -n '240,260p' feature/settings/src/commonMain/kotlin/com/mifos/feature/settings/settings/SettingsScreen.ktRepository: openMF/android-client
Length of output: 737
🏁 Script executed:
#!/bin/bash
# Check LanguageHelper.kt for locale management
cat core/common/src/commonMain/kotlin/com/mifos/core/common/utils/LanguageHelper.ktRepository: openMF/android-client
Length of output: 3210
🏁 Script executed:
#!/bin/bash
# Search for any locale provider or utility function
rg -n "fun.*[Ll]ocale\|object.*Locale\|class.*Locale" --type kotlin -A 3 | head -50Repository: openMF/android-client
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Check if there's a locale manager or preference
rg -n "preferences\|Preferences\|DataStore" --type kotlin | grep -i locale | head -20Repository: openMF/android-client
Length of output: 47
Use device locale instead of hardcoded "en" for API requests.
The locale is hardcoded to "en" in 14+ locations across the codebase, which means non-English users will receive English-formatted API responses regardless of their device locale settings. This undermines any i18n efforts and breaks API contract assumptions.
Replace hardcoded locale with the device's default language:
RecurringDepositApproval(
locale = Locale.getDefault().language,
dateFormat = "dd MMMM yyyy",
approvedOnDate = DateHelper.getDateAsStringForApproval(approvalDate),
note = reasonForApproval,
)For Kotlin Multiplatform projects, consider using expect/actual to abstract platform-specific locale retrieval if Locale.getDefault() is unavailable in commonMain.
This is a systemic issue affecting offline sync, savings, loans, groups, centers, and other features. Standardize on device locale across all API payload constructors.
🤖 Prompt for AI Agents
In
`@feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/recurringAccountApproval/RecurringDepositAccountApprovalScreen.kt`
around lines 260 - 267, The RecurringDepositApproval payload is using a
hardcoded locale "en" in RecurringDepositAccountApprovalScreen where
approveAccount.invoke(...) is called; replace that literal with the device
default language (e.g., Locale.getDefault().language) in the
RecurringDepositApproval constructor (keep dateFormat and approvedOnDate as-is
using DateHelper.getDateAsStringForApproval(approvalDate) and note =
reasonForApproval). If Locale.getDefault() is not available in commonMain,
implement an expect/actual platform accessor (e.g., getDeviceLanguage()) and use
that instead; apply the same replacement pattern to other payload constructors
across the codebase where "en" is hardcoded.
|
I've fetched the changes and tested locally. Looks good to me! |
core/network/src/commonMain/kotlin/com/mifos/core/network/services/RecurringAccountService.kt
Show resolved
Hide resolved
|
|
||
| viewModel { | ||
| RecurringDepositAccountApprovalViewModel( | ||
| savedStateHandle = get(), | ||
| approveRecurringDepositUseCase = get(), | ||
| ) | ||
| } |
There was a problem hiding this comment.
Don't provide viewmodel like this, see above for reference
| factory { | ||
| ApproveRecurringDepositUseCase( | ||
| repository = get(), | ||
| ) |
There was a problem hiding this comment.
RecurringDepositModule is for viewmodel only.
Use cases should not be here; see other use cases and where we provided them
|
@ufuomaisaac, I see we have a new screen addition in your PR, we didn't have that screen before? |
|
No, the screen wasn't there before @itsPronay Thank you for the review. I will make changes and update you. |
|
Thank you for the review! @techsavvy185 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@core/network/src/commonMain/kotlin/com/mifos/core/network/services/RecurringAccountService.kt`:
- Around line 37-41: The POST mapping in approveRecurringDepositAccount uses a
hardcoded path string; replace it with the APIEndPoint constant to match other
methods. Update the Retrofit annotation on approveRecurringDepositAccount to
reference APIEndPoint.CREATE_RECURRING_DEPOSIT_ACCOUNTS (and append the query
?command=approve as needed) so the service uses the shared endpoint constant
instead of the literal "recurringdepositaccounts/{accountId}" string.
🧹 Nitpick comments (2)
core/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerRecurringAccount.kt (1)
56-71: Extract a sharedJsoninstance to avoid repeated allocation.
Json { ignoreUnknownKeys = true }is instantiated on every call in bothcreateRecurringDepositAccount(line 40) and here (line 69). Allocating aJsoninstance is not free — the builder configures a serializers module each time. Hoist it to a class-levelvalor companion object.♻️ Proposed refactor
Add a companion (or private val) at the top of the class and reuse it:
class DataManagerRecurringAccount( val mBaseApiManager: BaseApiManager, ) { + private val json = Json { ignoreUnknownKeys = true } + fun createRecurringDepositAccount( ... ): Flow<GenericResponse> { ... - val json = Json { ignoreUnknownKeys = true } - - json.decodeFromString<GenericResponse>(response.bodyAsText()) + this@DataManagerRecurringAccount.json.decodeFromString<GenericResponse>(response.bodyAsText()) } } ... suspend fun approveRecurringDepositAccount( ... ): GenericResponse { ... - val json = Json { ignoreUnknownKeys = true } - return json.decodeFromString<GenericResponse>(response.bodyAsText()) + return json.decodeFromString<GenericResponse>(response.bodyAsText()) } }core/data/src/commonMain/kotlin/com/mifos/core/data/repository/RecurringAccountRepository.kt (1)
30-33: Inconsistent return-type convention within the repository interface.The existing methods return
Flow<DataState<...>>, while this new method is a rawsuspend funreturningGenericResponse. The Flow/DataState wrapping is deferred to the use-case layer instead. This works, but future contributors may find the mixed contract confusing.Consider either aligning this method with the existing
Flow<DataState<...>>pattern, or leaving a brief KDoc comment on the method explaining why it differs.
core/network/src/commonMain/kotlin/com/mifos/core/network/services/RecurringAccountService.kt
Outdated
Show resolved
Hide resolved
|
@itsPronay, I have made all the requested changes. Please take a look at it when you have the time. |
core/common/src/commonMain/kotlin/com/mifos/core/common/utils/DateHelper.kt
Show resolved
Hide resolved
| clientRecurringDepositAccountDestination( | ||
| navController = navController, | ||
| navigateBack = navController::popBackStack, | ||
| {}, | ||
| {}, | ||
| onApproveAccount = { accountNumber -> | ||
| navController.navigateToRecurringDepositAccountApproval(accountNumber) | ||
| }, | ||
| onViewAccount = {}, | ||
| ) | ||
|
|
||
| recurringDepositAccountApprovalDestination( | ||
| navigateBack = navController::popBackStack, | ||
| ) |
...ain/kotlin/com/mifos/feature/client/recurringDepositAccount/RecurringDepositAccountScreen.kt
Outdated
Show resolved
Hide resolved
...ain/kotlin/com/mifos/feature/client/recurringDepositAccount/RecurringDepositAccountScreen.kt
Outdated
Show resolved
Hide resolved
...ain/kotlin/com/mifos/feature/client/recurringDepositAccount/RecurringDepositAccountScreen.kt
Outdated
Show resolved
Hide resolved
...eature/recurringDeposit/recurringAccountApproval/RecurringDepositAccountApprovalViewModel.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/recurringDepositAccount/RecurringDepositAccountScreen.kt`:
- Around line 157-163: The Column inside the MifosScaffold is ignoring the
scaffold's paddingValues; update the Column's modifier in
RecurringDepositAccountScreen so it consumes those insets (use
Modifier.fillMaxSize().padding(paddingValues) or apply the appropriate padding
values via paddingValues.calculateTopPadding()/calculateBottomPadding() as
needed) so content won't render under system/top/bottom bars; locate the
MifosScaffold usage and the Column declaration to apply the padding.
In
`@feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/recurringAccountApproval/RecurringDepositAccountApprovalScreen.kt`:
- Around line 114-129: The content's isLoading prop isn't being driven by the
parent state so the double-tap guard is ineffective; in
RecurringDepositAccountApprovalScreen, when building the UI for non-error
states, call RecurringDepositAccountApprovalContent with isLoading = (uiState is
RecurringDepositAccountApprovalUiState.ShowProgressbar) (and continue to pass
approveAccount), and if you still want a visible spinner show
MifosProgressIndicator as an overlay or conditional child while keeping the
content rendered with isLoading=true so the Save button (in
RecurringDepositAccountApprovalContent) is immediately disabled.
🧹 Nitpick comments (5)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/recurringDepositAccount/RecurringDepositAccountScreen.kt (2)
140-145:else -> nullis a no-op in a statement-positionwhen.This branch silently evaluates to
nullwhich is discarded. Useelse -> Unit(consistent with the pattern at line 258) or omit the branch if thewhenis already exhaustive.Proposed fix
- else -> null + else -> Unit
212-218: Consider replacing the!!with a safe call.Line 215 uses a non-null assertion on
lastActiveTransactionDateright after a null check. While safe at runtime, aletblock is more idiomatic and resilient to future refactoring.Proposed fix
- lastActive = if (recurringDeposit.status?.submittedAndPendingApproval == true) { - stringResource(Res.string.client_savings_pending_approval) - } else if (recurringDeposit.lastActiveTransactionDate != null) { - DateHelper.getDateAsString(recurringDeposit.lastActiveTransactionDate!!) - } else { - notAvailableText - }, + lastActive = when { + recurringDeposit.status?.submittedAndPendingApproval == true -> + stringResource(Res.string.client_savings_pending_approval) + recurringDeposit.lastActiveTransactionDate != null -> + DateHelper.getDateAsString(recurringDeposit.lastActiveTransactionDate) + else -> notAvailableText + },Note: If
lastActiveTransactionDateis typed as nullable, the Kotlin smart-cast inside awhenbranch after the!= nullcheck will handle it without!!.feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/recurringAccountApproval/RecurringDepositAccountApprovalViewModel.kt (2)
45-76: Redundant eager progress state update on line 46.
ShowProgressbaris set on line 46 before the coroutine collects, and then again on line 60 whenDataState.Loadingis emitted. The eager set on line 46 is fine for instant UI feedback, but theDataState.Loadingbranch (lines 59-63) is then redundant. Consider removing one or the other for clarity.
35-37:approveRecurringDepositApplicationis unused — remove or use as screen callback.The function is never called anywhere in the codebase. The screen directly invokes
viewModel.trySendAction(RecurringDepositAccountApprovalAction.Approve(it))instead, bypassing this wrapper method. Either remove the function or use it as the screen's callback to eliminate dead code.feature/recurringDeposit/src/commonMain/kotlin/com/mifos/feature/recurringDeposit/recurringAccountApproval/RecurringDepositAccountApprovalScreen.kt (1)
274-279: Consider usingsealed interfaceinstead ofsealed class.The UI state has no shared mutable state or constructor logic, so
sealed interfacewould be more idiomatic and consistent with theRecurringDepositAccountApprovalActiondefinition in the ViewModel file (which already usessealed interface).Proposed change
-sealed class RecurringDepositAccountApprovalUiState { - data object Initial : RecurringDepositAccountApprovalUiState() - data object ShowProgressbar : RecurringDepositAccountApprovalUiState() - data class ShowRecurringDepositAccountApprovedSuccessfully(val response: GenericResponse) : RecurringDepositAccountApprovalUiState() - data class ShowError(val message: String) : RecurringDepositAccountApprovalUiState() -} +sealed interface RecurringDepositAccountApprovalUiState { + data object Initial : RecurringDepositAccountApprovalUiState + data object ShowProgressbar : RecurringDepositAccountApprovalUiState + data class ShowRecurringDepositAccountApprovedSuccessfully(val response: GenericResponse) : RecurringDepositAccountApprovalUiState + data class ShowError(val message: String) : RecurringDepositAccountApprovalUiState +}
...ain/kotlin/com/mifos/feature/client/recurringDepositAccount/RecurringDepositAccountScreen.kt
Outdated
Show resolved
Hide resolved
...s/feature/recurringDeposit/recurringAccountApproval/RecurringDepositAccountApprovalScreen.kt
Show resolved
Hide resolved
|
4 participants
Fixes - Jira-#MIFOSAC-361
Didn't create a Jira ticket, click here to create new.
Please Add Screenshots If there are any UI changes.
|
trim.050A5CAE-8058-447A-B387-84C25843E0C6.MOV
Please make sure these boxes are checked before submitting your pull request - thanks!
Run the static analysis check
./gradlew checkorci-prepush.shto make sure you didn't break anythingIf you have multiple commits please combine them into one commit by squashing them.
Summary by CodeRabbit
New Features
UI/UX Improvements
Other