feat(loan): implement filter in loan accounts screen#2602
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 status-based loan filtering feature: a FilterBottomSheet UI on ClientLoanAccountsScreen, new filter actions/state and LoanStatusFilter enum in the ViewModel, selection-based filtering logic with unfiltered backup, and new localized strings for filter labels and statuses. Changes
Sequence DiagramsequenceDiagram
participant User
participant Screen as ClientLoanAccountsScreen
participant ViewModel as ClientLoanAccountsViewModel
participant State as ClientLoanAccountsState
User->>Screen: Tap filter icon
Screen->>Screen: Show FilterBottomSheet
User->>Screen: Toggle status checkbox
Screen->>ViewModel: Dispatch HandleFilterClick(status)
ViewModel->>ViewModel: Toggle selectedStatus<br/>Filter loanAccounts from unfilteredLoanAccounts
ViewModel->>State: Emit updated selectedStatus & loanAccounts
State-->>Screen: New state (filtered list)
Screen->>Screen: Update UI & show active badge
User->>Screen: Tap Clear button
Screen->>ViewModel: Dispatch ClearFilters
ViewModel->>ViewModel: Reset selectedStatus<br/>Restore unfilteredLoanAccounts
ViewModel->>State: Emit reset state
State-->>Screen: New state (all loans)
Screen->>Screen: Update UI, remove badge
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsScreen.kt`:
- Around line 292-298: Rename the typo'd parameter handelFilterClick to
handleFilterClick in the FilterBottomSheet function signature and update all
uses inside that function to refer to handleFilterClick; then update every call
site that passes or references that parameter (e.g., where FilterBottomSheet is
invoked and any related lambdas in ClientListScreen) to use the corrected name
so the symbol matches across definitions and usages.
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsViewModel.kt`:
- Around line 149-151: The inner variable `val status = loan.status` inside the
filter lambda in ClientLoanAccountsViewModel is shadowing the enclosing function
parameter `status: String`; rename the inner variable (e.g., to `loanStatus`)
and update all uses within the lambda (the filter condition and any comparisons)
so the lambda reads `val loanStatus = loan.status ?: return@filter false`
thereby avoiding shadowing and improving readability while preserving behavior.
🧹 Nitpick comments (2)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsScreen.kt (2)
305-311: Hardcoded strings should use string resources for localization.The status types and UI labels ("Filters", "Account Status", "Clear", "Apply") are hardcoded. For i18n support, extract these to string resources similar to other strings in the file.
♻️ Suggested approach
Add string resources in the appropriate resources file:
// In strings file feature_client_filter_title = "Filters" feature_client_account_status = "Account Status" feature_client_status_active = "Active" feature_client_status_pending = "Pending Approval" // etc.Then use
stringResource():Text( - text = "Filters", + text = stringResource(Res.string.feature_client_filter_title), style = MifosTypography.titleLargeEmphasized, color = MaterialTheme.colorScheme.primary, )Also applies to: 323-327, 355-358
291-292: Consider makingFilterBottomSheetprivate.This composable appears to be used only within this screen file. Making it
privatewould be consistent with other composables in this file (e.g.,ClientsAccountHeader,ClientLoanAccountsDialog) and prevent unintended external usage.♻️ Proposed fix
`@OptIn`(ExperimentalMaterial3Api::class) `@Composable` -fun FilterBottomSheet( +private fun FilterBottomSheet(
...rc/commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsScreen.kt
Outdated
Show resolved
Hide resolved
...commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsViewModel.kt
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
...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
...rc/commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsScreen.kt
Outdated
Show resolved
Hide resolved
...commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsViewModel.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/clientLoanAccounts/ClientLoanAccountsScreen.kt`:
- Around line 335-353: Replace the hard-coded English contentDescription values
for the two Icon composables with localized string resources: change the Icon
using MifosIcons.Redo (inside the IconButton that calls clearFilters() and
onDismissRequest()) to use stringResource(R.string.clear) and change the Icon
using MifosIcons.Check (inside the IconButton that calls onDismissRequest()) to
use stringResource(R.string.apply); then add corresponding entries ("clear" and
"apply") to the platform strings.xml so accessibility labels are localized.
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsViewModel.kt`:
- Around line 138-166: The filter currently compares localized display strings
in handleFilterClick (and similar logic around selectedStatus usage), so replace
the string-based selectedStatus with a Set of stable keys (e.g., a new
LoanStatusFilter enum/sealed class with values like ACTIVE, CLOSED,
PENDING_APPROVAL, WAITING_FOR_DISBURSAL, OVERPAID), update handleFilterClick to
toggle those enum values and filter unfilteredLoanAccounts by matching
loan.status boolean flags against the enum values (set loanAccounts =
filteredList), and update the UI to pass LoanStatusFilter values while rendering
labels via stringResource(...) so localization does not break filtering; also
apply the same enum-based replacements where selectedStatus is read/modified
elsewhere (the other occurrences mentioned).
...rc/commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsScreen.kt
Show resolved
Hide resolved
...commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsViewModel.kt
Outdated
Show resolved
Hide resolved
...rc/commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsScreen.kt
Outdated
Show resolved
Hide resolved
piyushs-05
force-pushed
the
feature/Implemented-filter-loan-accounts-screen
branch
from
eb70b97 to
9df4098
Compare
piyushs-05
force-pushed
the
feature/Implemented-filter-loan-accounts-screen
branch
2 times, most recently
from
58af90e to
7e606ba
Compare
...commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsViewModel.kt
Show resolved
Hide resolved
piyushs-05
force-pushed
the
feature/Implemented-filter-loan-accounts-screen
branch
from
7e606ba to
0259834
Compare
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/clientLoanAccounts/ClientLoanAccountsViewModel.kt (1)
117-124:⚠️ Potential issue | 🟠 MajorFilter state becomes inconsistent after refresh/search.
When
getLoanAccountssucceeds, it resets bothloanAccountsandunfilteredLoanAccountsto the full result set, butselectedStatusis not cleared. This means the filter chips will still appear selected in the UI while the list shows unfiltered data.Either clear
selectedStatushere, or re-apply the active filters to the new data.Proposed fix (clear filters on reload)
mutableStateFlow.update { it.copy( loanAccounts = loanAccounts, unfilteredLoanAccounts = loanAccounts, + selectedStatus = emptyList(), dialogState = null, isLoading = false, ) }
🧹 Nitpick comments (1)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsViewModel.kt (1)
138-168: Consider usingSet<LoanStatusFilter>instead ofList<LoanStatusFilter>.
selectedStatusrepresents a collection of unique selections.Setis semantically more appropriate, guarantees uniqueness without relying on the toggle guard, and makes the intent clearer. The+/-/inoperators work identically onSet.Diff across affected locations
// In ClientLoanAccountsState: - val selectedStatus: List<LoanStatusFilter> = emptyList(), + val selectedStatus: Set<LoanStatusFilter> = emptySet(), // In clearFilters(): - selectedStatus = emptyList(), + selectedStatus = emptySet(),
piyushs-05
force-pushed
the
feature/Implemented-filter-loan-accounts-screen
branch
from
0259834 to
8b1be9e
Compare
|
@piyushs-05 Please run |
piyushs-05
force-pushed
the
feature/Implemented-filter-loan-accounts-screen
branch
from
a6b83db to
d0bf28d
Compare
|
@piyushs-05 Have you updated the red dot on filter if any of the options are selected? If yes, then update the screen recording. |
|
@biplab1 not in this one sir . Just be sure i need to change the colour of filter icon if any of the filters is selected right ? |
piyushs-05
force-pushed
the
feature/Implemented-filter-loan-accounts-screen
branch
3 times, most recently
from
0649670 to
1f87381
Compare
|
@biplab1 @therajanmaurya i have updated the code to have red filter dot , please review it sir |
|
@biplab1 do final review |
piyushs-05
force-pushed
the
feature/Implemented-filter-loan-accounts-screen
branch
from
1f87381 to
bf006d5
Compare
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/clientLoanAccounts/ClientLoanAccountsViewModel.kt`:
- Around line 233-239: Change the enum LoanStatusFilter from public to internal
since it's only used within the clientLoanAccounts package; update the
declaration of LoanStatusFilter in ClientLoanAccountsViewModel.kt to use
internal visibility and ensure any usages in ClientLoanAccountsViewModel and
ClientLoanAccountsScreen continue to compile (no external references exist).
🧹 Nitpick comments (4)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsViewModel.kt (1)
172-193: Missing blank line betweenclearFilters()and the extension properties.There's no blank line separating the
clearFiltersfunction (closing brace on line 179) from theisActuallyClosedextension property (line 180), which reduces readability.Proposed fix
private fun clearFilters() { mutableStateFlow.update { it.copy( loanAccounts = it.unfilteredLoanAccounts, selectedStatus = emptySet(), ) } } + private val LoanStatusEntity.isActuallyClosed: Booleanfeature/client/src/commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsScreen.kt (3)
266-289: Hard-coded padding values for the filter-active badge indicator.The badge positioning uses magic numbers (
top = 12.dp, end = 16.dp,size = 8.dp). Consider extracting these to named constants or usingDesignTokenvalues for consistency with the rest of the codebase.
349-354: Clearing filters also dismisses the sheet — is this intended?The "Clear" button calls both
clearFilters()andonDismissRequest(), so tapping it clears all selections and closes the bottom sheet. If the user wants to clear and then reselect different filters, they'd need to reopen the sheet. Consider whether clear should only reset the checkboxes without dismissing.
392-401: Consider adding a click target on the entire row for better UX.Currently only the
Checkboxis clickable. Wrapping theRowwith aModifier.clickable(or usingtoggleablewithRole.Checkbox) would make the label text tappable too, which is both a better UX and an accessibility improvement.Suggested direction
Row( verticalAlignment = Alignment.CenterVertically, + modifier = Modifier + .fillMaxWidth() + .toggleable( + value = isChecked, + role = Role.Checkbox, + onValueChange = { handleFilterClick(status) }, + ), ) { Spacer(modifier = Modifier.width(DesignToken.padding.medium)) Checkbox( checked = isChecked, - onCheckedChange = { handleFilterClick(status) }, + onCheckedChange = null, ) Text(text = statusLabel) }
...commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsViewModel.kt
Show resolved
Hide resolved
biplab1
left a comment
biplab1
left a comment
There was a problem hiding this comment.
These are the statuses I can see on the web-app:
"status": {
"Active": "Active",
"Approved": "Approved",
"Closed": "Closed",
"Closed (obligations met)": "Closed (obligations met)",
"Closed (rescheduled)": "Closed (rescheduled)",
"Closed (written off)": "Closed (written off)",
"Overpaid": "Overpaid",
"Pending": "Pending",
"Rejected": "Rejected",
"Submitted and pending approval": "Submitted and pending approval",
"Transfer in progress": "Transfer in progress",
"Transfer on hold": "Transfer on hold",
"Withdrawn": "Withdrawn"
},
Ref: https://github.com/openMF/web-app/blob/dev/src/assets/translations/en-US.json
Search: "status": {
...commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsViewModel.kt
Outdated
Show resolved
Hide resolved
|
@piyushs-05 For now, please restrict the filters to these statuses: Active, Closed, Overpaid, Pending, so that we can merge this PR. Rest of the statuses can be added to the filter later on. |
piyushs-05
force-pushed
the
feature/Implemented-filter-loan-accounts-screen
branch
from
7e3353e to
bbaa45b
Compare
biplab1
changed the title
biplab1
changed the title
|
5 participants
Fixes - Jira-#646
Please Add Screenshots If there are any UI changes.
before_filter.mp4
filter_after.mp4
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