Approve button in recurring account Issue #2580
Conversation
📝 WalkthroughWalkthroughAdds an approval workflow for recurring deposit accounts: date-format helper change, new serializable approval model, network/service/datamanager + repository API, DI and use-case wiring, ViewModel, navigation and UI screen with date picker and strings, plus a minor Ktor JSON config tweak. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User / UI
participant Screen as RecurringDepositAccountApprovalScreen
participant VM as RecurringDepositAccountApprovalViewModel
participant UseCase as ApproveRecurringDepositUseCase
participant Repo as RecurringAccountRepository
participant Service as RecurringAccountService
participant API as Remote API
User->>Screen: pick date & enter note, tap Save
Screen->>VM: approveRecurringDepositApplication(accountId, approval)
VM->>VM: set 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 HttpResponse -> GenericResponse / error
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
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.
...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
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
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 |
2 participants
Fixes - Jira-#Issue_Number
Didn't create a Jira ticket, click here to create new.
Please Add Screenshots If there are any UI changes.
trim.DD8A9707-D371-437B-9E54-E1375B38FC0D.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
✏️ Tip: You can customize this high-level summary in your review settings.