Added Functional Filters & Export Button UI to Loan Transactions Screen#2600
Added Functional Filters & Export Button UI to Loan Transactions Screen#2600gurnoorpannu wants to merge 9 commits intoopenMF:developmentfrom
Conversation
📝 WalkthroughWalkthroughAdds four string resources and a filter UI for loan transactions: two toggle flags (hideReversed, hideAccruals), FilterBottomSheet, export action placeholder, ViewModel state/mutators, and filtered rendering with empty-state handling. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant UI as LoanTransactionsScreen
participant VM as LoanTransactionsViewModel
participant Repo as LoanTransactionsRepository
User->>UI: Open screen / tap filter or export
UI->>VM: request UI state (transactions, hide flags)
VM->>Repo: loadLoanTransaction() / refresh()
Repo-->>VM: return transactions
VM-->>UI: emit uiState + hide flags
UI->>UI: compute filteredTransactions (apply hide flags)
User->>UI: toggle hideReversed / hideAccruals
UI->>VM: setHideReversed(value) / setHideAccruals(value)
VM-->>UI: emit updated flags -> UI recomputes filteredTransactions
User->>UI: tap Export
UI-->>VM: onExportClick() (placeholder)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanTransaction/LoanTransactionsScreen.kt`:
- Around line 19-21: Reorder the Kotlin imports so they follow alphabetical
order to satisfy spotlessKotlinCheck: move
androidclient.feature.loan.generated.resources.feature_loan_export,
feature_loan_hide_accruals, and feature_loan_hide_reversed to appear after
feature_loan_break_down and before feature_loan_id (or run ./gradlew
:feature:loan:spotlessApply to auto-fix); ensure the import block around
LoanTransactionsScreen.kt reflects this ordering.
- Around line 160-168: The filtering in the remember block for
filteredTransactions should use the explicit accrual boolean on Type instead of
string matching; update the hideAccruals branch to check
transaction.type?.accrual == true (keep the Reversed string check as-is), so
modify the lambda that references transactions, hideAccruals and
transaction.type to return false when hideAccruals && transaction.type?.accrual
== true and otherwise preserve the existing logic.
🧹 Nitpick comments (1)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanTransaction/LoanTransactionsScreen.kt (1)
177-177: Export functionality is not yet implemented.The export button currently has a placeholder TODO. If this is intentional for this PR (UI-only as per the title), consider tracking the implementation work.
Would you like me to open an issue to track the export functionality implementation?
.../loan/src/commonMain/kotlin/com/mifos/feature/loan/loanTransaction/LoanTransactionsScreen.kt
Outdated
Show resolved
Hide resolved
.../loan/src/commonMain/kotlin/com/mifos/feature/loan/loanTransaction/LoanTransactionsScreen.kt
Show resolved
Hide resolved
|
Looks good to me |
|
@gurnoorpannu Could you please upload screen recording showing the actions of the filters? |
| hideAccruals = hideAccruals, | ||
| onHideReversedChange = { hideReversed = it }, | ||
| onHideAccrualsChange = { hideAccruals = it }, | ||
| onExportClick = { /* TODO: Export functionality */ }, |
There was a problem hiding this comment.
Please avoid adding inline comments.
@biplab1 I have uploaded the recording |
|
@gurnoorpannu Please refer to the Client flow for the placement and implementation of the filters. Also, if possible, place the export button with a icon by its side in the title bar, or if it does not fit, we can have only the icon. Choose an appropriate icon from the material theme icons. We should think mobile first as much as possible rather than imitating the web-app. |
|
@gurnoorpannu You can refer to https://github.com/openMF/android-client/blob/development/feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientsList/ClientListScreen.kt and https://github.com/openMF/android-client/blob/development/feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientsList/ClientListViewModel.kt. . I think handling the filtering logic in viewmodel and saving the filters as states would be better. |
|
@biplab1 sir , I have implemented requested ui changes and updated the screen recording for the ui |
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 (1)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanTransaction/LoanTransactionsViewModel.kt (1)
22-68:⚠️ Potential issue | 🟠 MajorMissing initial data load.
The ViewModel lacks an
initblock to triggerloadLoanTransaction()on creation. Without this, the screen will remain in the loading/empty state untilrefresh()is explicitly called from the UI.🔧 Proposed fix to add initial load
class LoanTransactionsViewModel( private val repository: LoanTransactionsRepository, savedStateHandle: SavedStateHandle, ) : ViewModel() { val loanId = savedStateHandle.toRoute<LoanTransactionScreenRoute>().loanAccountNumber private val _loanTransactionsUiState = MutableStateFlow<LoanTransactionsUiState>(LoanTransactionsUiState.ShowProgressBar) val loanTransactionsUiState: StateFlow<LoanTransactionsUiState> get() = _loanTransactionsUiState + init { + refresh() + } + private val _hideReversed = MutableStateFlow(false)
🤖 Fix all issues with AI agents
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanTransaction/LoanTransactionsScreen.kt`:
- Around line 101-122: The export icon's onExportClick in the
LoanTransactionsScreen call is a no-op; update the call site to invoke the
ViewModel export action (e.g., call viewModel.exportTransactions() or the
appropriate export method in LoanTransactionsViewModel) or conditionally
disable/hide the export button until implemented; locate the
LoanTransactionsScreen composable invocation and replace the onExportClick = {
/* UI-only, no logic */ } with onExportClick = { viewModel.exportTransactions(/*
params if needed */) } or wire it to the existing export method and ensure the
ViewModel exposes a suspend/flow-safe exportTransactions function to handle the
export flow.
🧹 Nitpick comments (2)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanTransaction/LoanTransactionsViewModel.kt (2)
47-51: Potential for concurrent collectors on repeatedrefresh()calls.Each
refresh()call launches a new coroutine that collects from the repository flow. If the flow is long-lived (e.g., observes database changes) orrefresh()is called rapidly, multiple collectors may run concurrently, causing redundant emissions and potential state overwrites.Consider canceling the previous job before starting a new one:
♻️ Proposed fix using a tracked Job
+import kotlinx.coroutines.Job + class LoanTransactionsViewModel( private val repository: LoanTransactionsRepository, savedStateHandle: SavedStateHandle, ) : ViewModel() { + + private var loadJob: Job? = null // ... existing code ... fun refresh() { + loadJob?.cancel() - viewModelScope.launch { + loadJob = viewModelScope.launch { loadLoanTransaction() } }
33-45: Filter state is not persisted toSavedStateHandle.The filter flags (
_hideReversed,_hideAccruals) are stored in plainMutableStateFlowand will be lost on process death or configuration changes. Per the PR discussion recommending alignment with the Client list pattern, consider persisting these flags viaSavedStateHandleto preserve user selections.♻️ Proposed approach using SavedStateHandle
- private val _hideReversed = MutableStateFlow(false) - val hideReversed: StateFlow<Boolean> get() = _hideReversed - - private val _hideAccruals = MutableStateFlow(false) - val hideAccruals: StateFlow<Boolean> get() = _hideAccruals - - fun setHideReversed(value: Boolean) { - _hideReversed.value = value - } - - fun setHideAccruals(value: Boolean) { - _hideAccruals.value = value - } + private companion object { + const val KEY_HIDE_REVERSED = "hideReversed" + const val KEY_HIDE_ACCRUALS = "hideAccruals" + } + + val hideReversed: StateFlow<Boolean> = savedStateHandle.getStateFlow(KEY_HIDE_REVERSED, false) + val hideAccruals: StateFlow<Boolean> = savedStateHandle.getStateFlow(KEY_HIDE_ACCRUALS, false) + + fun setHideReversed(value: Boolean) { + savedStateHandle[KEY_HIDE_REVERSED] = value + } + + fun setHideAccruals(value: Boolean) { + savedStateHandle[KEY_HIDE_ACCRUALS] = value + }
.../loan/src/commonMain/kotlin/com/mifos/feature/loan/loanTransaction/LoanTransactionsScreen.kt
Show resolved
Hide resolved
.../loan/src/commonMain/kotlin/com/mifos/feature/loan/loanTransaction/LoanTransactionsScreen.kt
Show resolved
Hide resolved
|
@biplab1 any further changes needed on this one? |
|
4 participants
Fixes - MIFOSAC-652
Before:-
After:-
transactionScreen_filters.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