fix(client): Added Create Account buttons for all types of accounts.#2583
fix(client): Added Create Account buttons for all types of accounts.#2583techsavvy185 wants to merge 9 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:
📝 WalkthroughWalkthroughThis PR adds account creation callback parameters and AddAccount event/action handling across five account modules (Loan, Fixed Deposit, Recurring Deposit, Savings, Share). Routes, screens, and view models are updated to support account creation from empty states, with UI components displaying AddAccount buttons and localized empty-state messages. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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: 3
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)
244-248: Pre-existing bug:ApproveAccountaction is not dispatched.The
ApproveAccountaction is constructed but never sent viaonAction(), so the approve functionality won't work. Compare withViewAccount(lines 237-243) which correctly callsonAction(...).🐛 Proposed fix
is Actions.ApproveAccount -> { - RecurringDepositAccountAction.ApproveAccount( + onAction(RecurringDepositAccountAction.ApproveAccount( recurringDeposit.accountNo ?: "", - ) + )) }
🤖 Fix all issues with AI agents
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsScreen.kt`:
- Around line 242-249: Replace the null content description on the add-icon
button so screen readers can announce it: in the Icon inside IconButton (the one
invoking onAction(ClientLoanAccountsAction.AddAccount) and using
painterResource(Res.drawable.add_icon)), set contentDescription to a meaningful
string (preferably via a string resource, e.g.
stringResource(R.string.add_account) or a localized label like "Add account")
instead of null so the button is accessible.
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/fixedDepositAccount/FixedDepositAccountScreen.kt`:
- Around line 13-14: The Add icon in FixedDepositAccountScreen (the clickable
add icon near where the composable shows an empty state) has a null
contentDescription, making it inaccessible; update the composable that renders
the add_icon (in FixedDepositAccountScreen) to pass a meaningful, localized
contentDescription string (create a new string resource like "add_fixed_deposit"
or reuse an appropriate existing resource) and wire it via stringResource(...)
so screen readers announce the action; ensure the contentDescription is non-null
for the Image/Icon composable and update any related preview/tests to reflect
the new resource.
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/savingsAccounts/SavingsAccounts.kt`:
- Around line 254-262: Remove the no-op DesignToken.padding expression and
replace it with a proper Spacer using Modifier.width or Modifier.padding as
appropriate (replace the standalone DesignToken.padding near the IconButton);
also fix accessibility by providing a meaningful contentDescription for the Icon
(do not use contentDescription = null) — either hardcode a short label like "Add
account" or reference a string resource, and ensure the IconButton callback
remains onAction.invoke(SavingsAccountAction.AddAccount).
🧹 Nitpick comments (1)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/shareAccounts/ShareAccountsScreen.kt (1)
186-202: Consider adding meaningfulcontentDescriptionfor accessibility.The new Add icon (and the existing Search/Filter icons) have
contentDescription = null, which makes them inaccessible to screen readers. For interactive icons, providing descriptive labels improves accessibility.♿ Suggested improvement for accessibility
Icon( modifier = Modifier.onClick { onAction.invoke(ShareAccountsAction.ToggleSearchBar) }, painter = painterResource(Res.drawable.search), - contentDescription = null, + contentDescription = "Search accounts", ) Spacer(modifier = Modifier.width(DesignToken.spacing.largeIncreased)) Icon( modifier = Modifier.onClick { onAction.invoke(ShareAccountsAction.AddAccount) }, painter = painterResource(Res.drawable.add_icon), - contentDescription = null, + contentDescription = "Add account", ) Spacer(modifier = Modifier.width(DesignToken.spacing.largeIncreased)) Icon( modifier = Modifier.onClick { onAction.invoke(ShareAccountsAction.ToggleFiler) }, painter = painterResource(Res.drawable.filter), - contentDescription = null, + contentDescription = "Filter accounts", )
...rc/commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsScreen.kt
Outdated
Show resolved
Hide resolved
.../commonMain/kotlin/com/mifos/feature/client/fixedDepositAccount/FixedDepositAccountScreen.kt
Outdated
Show resolved
Hide resolved
...ure/client/src/commonMain/kotlin/com/mifos/feature/client/savingsAccounts/SavingsAccounts.kt
Outdated
Show resolved
Hide resolved
techsavvy185
changed the title
niyajali
left a comment
niyajali
left a comment
There was a problem hiding this comment.
Do these require changes in all places
...src/commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsRoute.kt
Outdated
Show resolved
Hide resolved
...rc/commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsScreen.kt
Outdated
Show resolved
Hide resolved
...rc/commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsScreen.kt
Outdated
Show resolved
Hide resolved
|
@techsavvy185 could you also post images/videos of the outcome |
...rc/commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsScreen.kt
Outdated
Show resolved
Hide resolved
|
@niyajali Thanks for the review! I'll make the required changes. I've already attached a screen recording in the PR, is there anything specific I should cover? |
There was a problem hiding this comment.
Actionable comments posted: 0
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)
270-310: Add an accessible contentDescription for the new add button.The new clickable icon has
contentDescription = null, so screen readers can’t discover or announce the action. Please provide a localized label.♿ Suggested fix
Icon( painter = painterResource(Res.drawable.add_icon), - contentDescription = null, + contentDescription = stringResource(Res.string.client_add_account), modifier = Modifier.clickable { addAccount.invoke() }, )
🧹 Nitpick comments (4)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/shareAccounts/ShareAccountsViewModel.kt (1)
36-36: Minor style inconsistency:iskeyword used for data object.Other
data objectactions in thiswhenblock (e.g.,ToggleFiler,ToggleSearchBar,Refresh) use direct matching withoutis. For consistency, consider removing theiskeyword here.Suggested change
- is ShareAccountsAction.AddAccount -> sendEvent(ShareAccountsEvent.AddAccount(route.clientId)) + ShareAccountsAction.AddAccount -> sendEvent(ShareAccountsEvent.AddAccount(route.clientId))feature/client/src/commonMain/kotlin/com/mifos/feature/client/savingsAccounts/SavingsAccountsRoute.kt (1)
35-35: Simplify the pass-through lambda.The lambda wrapping is unnecessary since it just forwards the argument directly.
♻️ Suggested simplification
- createAccount = { clientId -> createAccount(clientId) }, + createAccount = createAccount,feature/client/src/commonMain/kotlin/com/mifos/feature/client/fixedDepositAccount/FixedDepositAccountScreen.kt (1)
267-274: Consider reordering parameters for consistency.The
addAccountparameter (without default) is placed aftermodifier(with default). While Kotlin's named arguments make this work, grouping parameters without defaults before those with defaults improves readability and aligns with common Compose conventions.♻️ Suggested parameter order
`@Composable` fun FixedDepositAccountHeader( totalItem: String, onToggleFilter: () -> Unit, + addAccount: () -> Unit, onToggleSearch: () -> Unit, modifier: Modifier = Modifier, - addAccount: () -> Unit, - onToggleSearch: () -> Unit, )feature/client/src/commonMain/kotlin/com/mifos/feature/client/shareAccounts/ShareAccountsScreen.kt (1)
192-196: Consider addingcontentDescriptionfor accessibility.Setting
contentDescription = nullmeans screen readers cannot describe this button. For better accessibility, provide a meaningful description.This also applies to the existing search and filter icons in this header.
♻️ Suggested improvement
Icon( modifier = Modifier.onClick { onAction.invoke(ShareAccountsAction.AddAccount) }, painter = painterResource(Res.drawable.add_icon), - contentDescription = null, + contentDescription = "Add Share Account", )
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 12-21: The icon usage (add_icon) in RecurringDepositAccountScreen
currently sets contentDescription = null and always renders a Spacer after the
icon, which removes the action from screen readers and leaves a gap when the
icon is hidden; update the composable(s) that render add_icon (e.g., the
icon/Image composable and its surrounding layout in
RecurringDepositAccountScreen and the similar block around the code referenced
at lines ~307-318) to: 1) provide a meaningful contentDescription using an
accessible string resource (e.g.,
Res.client_profile_recurring_deposit_account_title or a dedicated "add" label)
instead of null, and 2) render the Spacer conditionally only when the icon is
actually shown (wrap the Spacer in the same if that shows the icon) so no extra
gap appears when the icon is omitted. Ensure the accessible label is localized
via the existing resources import.
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/shareAccounts/ShareAccountsScreen.kt`:
- Around line 12-17: The use of add_icon currently sets contentDescription =
null (making it invisible to screen readers) and always renders a Spacer even
when the icon is hidden; update the Icon/Image usage for add_icon in
ShareAccountsScreen (and the similar nodes around the 192-206 block) to provide
an accessible label by passing a meaningful string resource (e.g., from Res like
feature_share_account_empty_list_message or a new localized string such as "add
share" via Res) into contentDescription instead of null, and render the Spacer
conditionally only when the add_icon is actually visible (wrap Spacer rendering
with the same visibility condition used for the icon).
🧹 Nitpick comments (1)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/fixedDepositAccount/FixedDepositAccountScreen.kt (1)
301-328: Consider aligning icon button patterns with SavingsAccountsHeader.The header icons use
Iconwithclickablemodifier, whileSavingsAccountsHeaderusesIconButton. For consistency across the codebase and better accessibility (proper touch targets), consider refactoring all interactive icons to useIconButton.Also note that search icon (line 303) and filter icon (line 324) have
contentDescription = null, though these lines aren't part of this PR's changes.
...ain/kotlin/com/mifos/feature/client/recurringDepositAccount/RecurringDepositAccountScreen.kt
Show resolved
Hide resolved
...e/client/src/commonMain/kotlin/com/mifos/feature/client/shareAccounts/ShareAccountsScreen.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
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)
249-255: Bug:ApproveAccountaction is created but never dispatched.The
ApproveAccountaction is instantiated but not passed toonAction(), so clicking "Approve Account" does nothing. Compare withViewAccountat line 243 which correctly callsonAction(...).🐛 Proposed fix
is Actions.ApproveAccount -> { - RecurringDepositAccountAction.ApproveAccount( + onAction( + RecurringDepositAccountAction.ApproveAccount( - recurringDeposit.accountNo ?: "", + recurringDeposit.accountNo ?: "", + ), ) }
🧹 Nitpick comments (3)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/recurringDepositAccount/RecurringDepositAccountScreen.kt (1)
66-74: Parameter ordering violates Compose conventions.The
createAccountparameter is required (no default value) but is placed aftermodifierwhich has a default. Compose API guidelines recommend placing required parameters before optional ones, withmodifiertypically being the last optional parameter.♻️ Suggested fix
`@Composable` fun RecurringDepositAccountScreen( navController: NavController, navigateBack: () -> Unit, onApproveAccount: (String) -> Unit, onViewAccount: (String) -> Unit, + createAccount: (Int) -> Unit, modifier: Modifier = Modifier, - createAccount: (Int) -> Unit, viewModel: RecurringDepositAccountViewModel = koinViewModel(), ) {feature/client/src/commonMain/kotlin/com/mifos/feature/client/fixedDepositAccount/FixedDepositAccountScreen.kt (2)
66-74: Consider reordering parameters for better API consistency.The
createAccountparameter (required, no default) is placed aftermodifier(optional with default). Kotlin/Compose conventions typically place required parameters before optional ones. Consider movingcreateAccountbeforemodifierfor consistency.♻️ Suggested reordering
`@Composable` fun FixedDepositAccountScreen( navController: NavController, navigateBack: () -> Unit, onApproveAccount: (String) -> Unit, onViewAccount: (String) -> Unit, + createAccount: (Int) -> Unit, modifier: Modifier = Modifier, - createAccount: (Int) -> Unit, viewModel: FixedDepositAccountViewModel = koinViewModel(), )
273-281: Consider consistent parameter ordering in header function.Required parameters (
onToggleFilter,addAccount,onToggleSearch,isFixedDepositScreenEmpty) are interleaved around the optionalmodifierparameter. For consistency, group all required parameters before optional ones.♻️ Suggested reordering
`@Composable` fun FixedDepositAccountHeader( totalItem: String, onToggleFilter: () -> Unit, + addAccount: () -> Unit, onToggleSearch: () -> Unit, + isFixedDepositScreenEmpty: Boolean, modifier: Modifier = Modifier, - addAccount: () -> Unit, - onToggleSearch: () -> Unit, - isFixedDepositScreenEmpty: Boolean, )
|
@niyajali I'm done with all the changes that were required. Could you please go through this? |
|
@techsavvy185 Please resolve the merge conflicts. |
|
@biplab1 Yes, I'll complete it by tomorrow. |
techsavvy185
force-pushed
the
createAccountFlow
branch
from
26cc410 to
10addb6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/recurringDepositAccount/RecurringDepositAccountScreen.kt (1)
239-243:⚠️ Potential issue | 🔴 CriticalBug:
ApproveAccountaction is constructed but never dispatched.The action is created but not passed to
onAction(...), so clicking "Approve" on a recurring deposit account will silently do nothing. Compare withFixedDepositAccountScreen.kt(lines 241–247) where it's correctly wrapped inonAction(...).Proposed fix
is Actions.ApproveAccount -> { - RecurringDepositAccountAction.ApproveAccount( + onAction(RecurringDepositAccountAction.ApproveAccount( recurringDeposit.accountNo ?: "", - ) + )) }feature/client/src/commonMain/kotlin/com/mifos/feature/client/navigation/ClientNavigation.kt (1)
362-365:⚠️ Potential issue | 🟡 Minor
shareAccountsDestinationhasnavigateToViewAccountwired to a no-op{}.While
createAccountis correctly handled internally inShareAccountsRoute, thenavigateToViewAccountcallback is still an empty lambda. This means the "View Account" action on share account cards will silently do nothing. Is this intentional / pending implementation?feature/client/src/commonMain/kotlin/com/mifos/feature/client/shareAccounts/ShareAccountsScreen.kt (1)
52-57:⚠️ Potential issue | 🔴 CriticalRename
ShareAccountsScreentoShareAccountsScreenRouteto fix compilation error.ShareAccountsRoute.kt line 28 calls
ShareAccountsScreenRoute(...), but ShareAccountsScreen.kt definesShareAccountsScreen. This function name mismatch will cause a compilation error. Rename the function at line 53 fromShareAccountsScreentoShareAccountsScreenRoute.Additionally, fix accessibility issues: The three icons in ShareAccountHeader (lines 193, 199, 205) all have
contentDescription = null. Provide descriptive content descriptions for search, add, and filter icons.
🤖 Fix all issues with AI agents
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/savingsAccounts/SavingsAccountsScreen.kt`:
- Around line 242-272: DesignToken.padding expressions in the conditional block
inside SavingsAccountsScreen (around the IconButton calls that invoke
SavingsAccountAction.ToggleSearch, AddAccount, and ToggleFilter) are no-ops and
should be replaced with real spacing or removed; update those two occurrences to
use a Spacer with Modifier.width(DesignToken.spacing.largeIncreased) between the
icons (matching ShareAccountsScreen.kt) or simply delete the unused
DesignToken.padding lines so the IconButtons remain adjacent but valid touch
targets.
🧹 Nitpick comments (3)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/fixedDepositAccount/FixedDepositAccountScreen.kt (1)
65-73: Required parametercreateAccountplaced after optional parametermodifier.Placing a required parameter (
createAccount) after one with a default value (modifier) forces all callers to use named arguments. MovecreateAccountbeforemodifierto follow Kotlin/Compose conventions.Proposed fix
fun FixedDepositAccountScreen( navController: NavController, navigateBack: () -> Unit, onApproveAccount: (String) -> Unit, onViewAccount: (String) -> Unit, - modifier: Modifier = Modifier, createAccount: (Int) -> Unit, + modifier: Modifier = Modifier, viewModel: FixedDepositAccountViewModel = koinViewModel(), )feature/client/src/commonMain/kotlin/com/mifos/feature/client/recurringDepositAccount/RecurringDepositAccountScreen.kt (1)
65-73: Required parametercreateAccountplaced after optional parametermodifier.Same issue as in
FixedDepositAccountScreen— movecreateAccountbeforemodifier.Proposed fix
fun RecurringDepositAccountScreen( navController: NavController, navigateBack: () -> Unit, onApproveAccount: (String) -> Unit, onViewAccount: (String) -> Unit, - modifier: Modifier = Modifier, createAccount: (Int) -> Unit, + modifier: Modifier = Modifier, viewModel: RecurringDepositAccountViewModel = koinViewModel(), )feature/client/src/commonMain/kotlin/com/mifos/feature/client/savingsAccounts/SavingsAccountsRoute.kt (1)
35-35: Nit: Redundant lambda wrapper.
{ clientId -> createAccount(clientId) }can be simplified to a direct function reference.Proposed fix
- createAccount = { clientId -> createAccount(clientId) }, + createAccount = createAccount,
...ient/src/commonMain/kotlin/com/mifos/feature/client/savingsAccounts/SavingsAccountsScreen.kt
Show resolved
Hide resolved
|
4 participants
Fixes - Jira-#629
Screen_recording_20260127_184125.webm
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
Release Notes