feat(loan): add export transactions dialog to loan transactions screen#2607
feat(loan): add export transactions dialog to loan transactions screen#2607gurnoorpannu wants to merge 42 commits into
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 an export feature: seven new string resources, a new ExportTransactionsDialog composable with from/to date pickers and range validation, and an export IconButton in LoanTransactionsScreen that opens the dialog and returns a selected date range. Changes
Sequence DiagramsequenceDiagram
participant User
participant LTScreen as LoanTransactionsScreen
participant Dialog as ExportTransactionsDialog
participant Picker as DatePicker
User->>LTScreen: Tap export button
LTScreen->>LTScreen: set showExportDialog = true
LTScreen->>Dialog: render dialog
User->>Dialog: Open from-date picker
Dialog->>Picker: request from-date (min 2000-01-01)
Picker->>Dialog: return fromDate (ms)
Dialog->>Dialog: update fromDate
User->>Dialog: Open to-date picker
Dialog->>Picker: request to-date (min 2000-01-01)
Picker->>Dialog: return toDate (ms)
Dialog->>Dialog: update toDate, validate range
alt Valid date range
User->>Dialog: Tap Generate Report
Dialog->>LTScreen: onGenerateReport(fromMs, toMs)
LTScreen->>Dialog: dismiss dialog
else Invalid date range
Dialog->>User: show invalid date range error
end
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 |
|
I think the the close(x) button should be black with transparent background. |
| private fun formatDateFromMillis(millis: Long?): String { | ||
| if (millis == null) return "" | ||
| val localDate = Instant.fromEpochMilliseconds(millis) | ||
| .toLocalDateTime(TimeZone.currentSystemDefault()) |
There was a problem hiding this comment.
there is a time zone mismatch with fun createSelectableDatesFrom(minDate: LocalDate) = object : SelectableDates I suggest using TimeZone.UTC for consistency
There was a problem hiding this comment.
will fix that, ty for the review :)
|
|
||
| <!-- Export Loan Transactions --> | ||
| <string name="feature_loan_export">Export</string> | ||
| <string name="feature_loan_export_transactions">Export</string> |
There was a problem hiding this comment.
why two different keys with same values ? you meant the second one to be Export Transactions did't you
| } | ||
|
|
||
| @OptIn(ExperimentalTime::class) | ||
| @Composable |
There was a problem hiding this comment.
I think this function should not be annotated with Composable it does not emit UI, call remember or other compose APIs or take compose parameter, marking it with compose it triggers unnecessary recomposition tracking overhead.
| var showToDatePicker by rememberSaveable { mutableStateOf(false) } | ||
| var fromDate: Long? by rememberSaveable { mutableStateOf(null) } | ||
| var toDate: Long? by rememberSaveable { mutableStateOf(null) } | ||
| var showInvalidDateRangeError by remember { mutableStateOf(false) } |
There was a problem hiding this comment.
this will be lost on config change, should not it be rememberSaveable also ?
There was a problem hiding this comment.
@markrizkalla I kept it as remember intentionally since it is just a error message that gets recalculated when the user interacts with the form only the actual date values need to survive config changes
gurnoorpannu
force-pushed
the
feature/loan-transactions-export-dialog
branch
from
377ec90 to
c7cb0e4
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
feature/loan/src/commonMain/composeResources/values/strings.xml (1)
297-298: Duplicate string values:feature_loan_exportandfeature_loan_export_transactionsare both "Export".These strings serve different UI purposes:
feature_loan_exportis used as a content description for the export button icon (line 136 of LoanTransactionsScreen.kt), whilefeature_loan_export_transactionsis the dialog title (line 134 of ExportTransactionsDialog.kt). Consider differentiating their values—e.g., "Export" for the button and "Export Transactions" for the dialog title—to provide better context in different parts of the UI.
There was a problem hiding this comment.
Hey @gurnoorpannu ! Consider attaching a video (screen recording) for all the possible situations as mentioned in the ticket description-
On tapping Export, show a dialog with:
From Date (required)
To Date (required)
Cancel button
Generate Report button (disabled until validation passes)
Validation rules:
Both dates must be selected
To Date cannot be earlier than From Date
Only after valid input, the Generate Report button should become active
It's hard to validate these things with a single screenshot.
Thanks!
updated it 👍 |
|
@gurnoorpannu Have you validated that if |
| return selectedDate >= minDate | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
If you can, try to move this composable to core/ui module so that it can be reused.
We use date pickers in lot of places.
@amanna13 the logic is there for the dates. And for the Generate button , is asked @biplab1 sir for that as generate Report was breaking the ui |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanTransaction/ExportTransactionsDialog.kt (1)
185-197: Replace non-null assertions with a safe-call to avoid any future footgun.
fromDate!!andtoDate!!are safe today because the button is onlyenabledwhenisValidDateRangeistrue, which guards both non-null. However, the correctness of the call site depends entirely on theenabledstate remaining in sync with the null-checks. Alet-based pattern makes the contract explicit without any runtime cost.♻️ Proposed refactor
- onClick = { - onGenerateReport(fromDate!!, toDate!!) - }, + onClick = { + val from = fromDate + val to = toDate + if (from != null && to != null) { + onGenerateReport(from, to) + } + },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanTransaction/ExportTransactionsDialog.kt` around lines 185 - 197, Replace the unsafe non-null assertions in the MifosButton onClick (currently calling onGenerateReport(fromDate!!, toDate!!)) with a safe-call/let pattern that only invokes onGenerateReport when both fromDate and toDate are non-null; update the onClick in ExportTransactionsDialog (the MifosButton handler) to check fromDate and toDate via safe calls (e.g., nested let or a combined null-check) before calling onGenerateReport so the call no longer relies on !! while preserving the enabled/isValidDateRange behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanTransaction/ExportTransactionsDialog.kt`:
- Around line 128-136: Replace the bare Icon(...) with an IconButton that wraps
the MifosIcons.Cancel icon and calls onDismiss() (use IconButton { Icon(...) }
or the Compose equivalent) so the touch target meets the 48×48 dp minimum;
change the icon's contentDescription from the shared feature_loan_cancel string
to a dedicated string resource (e.g., feature_loan_close) so screen readers
distinguish the close icon from the Cancel button below, and add that new string
resource ("Close"/"Dismiss") accordingly.
- Around line 223-231: The current createSelectableDatesFrom implementation only
overrides isSelectableDate, causing the year spinner to allow navigation before
minDate and permitting future dates; update the returned SelectableDates
anonymous object to also override isSelectableYear(year: Int): Boolean to return
year in minDate.year..today.year (compute today via
Clock.System.now()/Instant/LocalDate with TimeZone.UTC), and modify
isSelectableDate to also cap the upper bound by today (i.e., return selectedDate
>= minDate && selectedDate <= today) so both individual dates and year
navigation are correctly constrained; refer to createSelectableDatesFrom,
isSelectableDate, isSelectableYear, LocalDate and TimeZone.UTC to locate the
changes.
- Around line 56-62: The file imports the wrong Instant
(kotlinx.datetime.Instant) which is incompatible with kotlinx-datetime 0.7.1;
remove the line importing kotlinx.datetime.Instant and replace it with import
kotlin.time.Instant so the Instant.fromEpochMilliseconds() calls in
ExportTransactionsDialog (and related uses alongside kotlin.time.Clock) compile
correctly.
---
Nitpick comments:
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanTransaction/ExportTransactionsDialog.kt`:
- Around line 185-197: Replace the unsafe non-null assertions in the MifosButton
onClick (currently calling onGenerateReport(fromDate!!, toDate!!)) with a
safe-call/let pattern that only invokes onGenerateReport when both fromDate and
toDate are non-null; update the onClick in ExportTransactionsDialog (the
MifosButton handler) to check fromDate and toDate via safe calls (e.g., nested
let or a combined null-check) before calling onGenerateReport so the call no
longer relies on !! while preserving the enabled/isValidDateRange behavior.
…-dialog' into feature/loan-transactions-export-dialog
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
feature/loan/src/commonMain/composeResources/values/strings.xml (1)
297-298:feature_loan_exportandfeature_loan_export_transactionshave identical values.Both resolve to
"Export". If the dialog title ever needs to read "Export Transactions" (which is common UX), the strings will need to be separated anyway. Consider differentiating them now.♻️ Suggested rename
- <string name="feature_loan_export_transactions">Export</string> + <string name="feature_loan_export_transactions">Export Transactions</string>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/loan/src/commonMain/composeResources/values/strings.xml` around lines 297 - 298, The two string resources feature_loan_export and feature_loan_export_transactions both have the same value "Export", which prevents using a distinct dialog title like "Export Transactions"; update feature_loan_export_transactions to a more specific label such as "Export Transactions" (or another appropriate differentiated text) and ensure any callers that expect the dialog title use feature_loan_export_transactions, leaving feature_loan_export for the generic export label; also update any localization entries that mirror these keys.feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanTransaction/ExportTransactionsDialog.kt (3)
185-187: Replace!!assertions with a safeletcall.
fromDate!!andtoDate!!are guarded byisValidDateRange, so they cannot NPE at runtime, but non-null assertions are generally fragile if the guard condition changes in the future.♻️ Proposed refactor
- onClick = { - onGenerateReport(fromDate!!, toDate!!) - }, + onClick = { + val from = fromDate + val to = toDate + if (from != null && to != null) { + onGenerateReport(from, to) + } + },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanTransaction/ExportTransactionsDialog.kt` around lines 185 - 187, The onClick currently calls onGenerateReport(fromDate!!, toDate!!) using non-null assertions which is fragile; instead, guard the call with a safe let so the report is only generated when both dates are non-null. In ExportTransactionsDialog, replace the forced unwraps by checking the same condition you use for enabling (isValidDateRange) or use fromDate?.let { f -> toDate?.let { t -> onGenerateReport(f, t) } } so onGenerateReport is invoked only with non-null f and t.
114-116:Boxwrapper has no effect and can be removed.
Box(contentAlignment = Alignment.Center)only makes sense when children overlap. Here it wraps a singleColumnchild; thecontentAlignmentis ignored, and theBoxadds a pointless layout node.♻️ Proposed fix
- Box( - contentAlignment = Alignment.Center, - ) { - Column(modifier = Modifier.padding(20.dp)) { + Column(modifier = Modifier.padding(20.dp)) {Also remove the corresponding closing
}for theBox.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanTransaction/ExportTransactionsDialog.kt` around lines 114 - 116, Remove the unnecessary Box wrapper in ExportTransactionsDialog: delete the Box(contentAlignment = Alignment.Center, ) block and its matching closing brace so the inner Column becomes the direct child of the parent composable; this removes a pointless layout node since Box's contentAlignment is unused when only a single Column child is present (search for Box(...) and the nested Column in ExportTransactionsDialog to locate the code).
83-86:toDatePickerStateminimum date doesn't reflect the selectedfromDate.Both picker states hardcode
2000-01-01as the minimum, so the "To Date" picker allows selecting any date from 2000 onwards regardless of whatfromDateis set to. Users can pick an invalid range in the picker itself, with only a post-selection error message catching it. Reactively updatingtoDatePickerState's minimum to matchfromDatewould prevent the invalid selection entirely.♻️ Suggested UX improvement
- val toDatePickerState = rememberDatePickerState( - initialSelectedDateMillis = Clock.System.now().toEpochMilliseconds(), - selectableDates = createSelectableDatesFrom(LocalDate.parse("2000-01-01")), - ) + val toDatePickerState = key(fromDate) { + rememberDatePickerState( + initialSelectedDateMillis = fromDate ?: Clock.System.now().toEpochMilliseconds(), + selectableDates = createSelectableDatesFrom( + minDate = fromDate + ?.let { Instant.fromEpochMilliseconds(it).toLocalDateTime(TimeZone.UTC).date } + ?: LocalDate.parse("2000-01-01"), + ), + ) + }Note: wrapping
rememberDatePickerStateinkey(fromDate)recreates the picker state wheneverfromDatechanges, sotoDatewill also be reset whenfromDatechanges. Add handling if you want to preserve a previously validtoDateselection.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanTransaction/ExportTransactionsDialog.kt` around lines 83 - 86, The To-date picker currently hardcodes the minimum as "2000-01-01" so users can pick a to-date earlier than the selected from-date; update the toDatePickerState to react to the selected fromDate by recreating it with the appropriate selectableDates (use createSelectableDatesFrom(LocalDate.parse(fromDate)) or equivalent) so the minimum equals the chosen fromDate; implement this by wrapping the rememberDatePickerState call for toDatePickerState in key(fromDate) and constructing selectableDates from the parsed fromDate so the picker prevents invalid ranges (note you may need to add handling to preserve an existing valid toDate if desired).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanTransaction/ExportTransactionsDialog.kt`:
- Around line 191-195: The "Generate Report" label in ExportTransactionsDialog
can truncate on small screens or large font scales because the MifosButton (in
ExportTransactionsDialog) is given weight(1f) and the Text composable uses
maxLines = 1; fix by either shortening the label (change the stringResource used
by the Text in the MifosButton to "Generate") or allow wrapping by changing the
Text in ExportTransactionsDialog to maxLines = 2 (and optionally set overflow =
TextOverflow.Ellipsis) so the button text doesn't clip under increased font
scales; update the Text inside the MifosButton in ExportTransactionsDialog
accordingly.
---
Nitpick comments:
In `@feature/loan/src/commonMain/composeResources/values/strings.xml`:
- Around line 297-298: The two string resources feature_loan_export and
feature_loan_export_transactions both have the same value "Export", which
prevents using a distinct dialog title like "Export Transactions"; update
feature_loan_export_transactions to a more specific label such as "Export
Transactions" (or another appropriate differentiated text) and ensure any
callers that expect the dialog title use feature_loan_export_transactions,
leaving feature_loan_export for the generic export label; also update any
localization entries that mirror these keys.
In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanTransaction/ExportTransactionsDialog.kt`:
- Around line 185-187: The onClick currently calls onGenerateReport(fromDate!!,
toDate!!) using non-null assertions which is fragile; instead, guard the call
with a safe let so the report is only generated when both dates are non-null. In
ExportTransactionsDialog, replace the forced unwraps by checking the same
condition you use for enabling (isValidDateRange) or use fromDate?.let { f ->
toDate?.let { t -> onGenerateReport(f, t) } } so onGenerateReport is invoked
only with non-null f and t.
- Around line 114-116: Remove the unnecessary Box wrapper in
ExportTransactionsDialog: delete the Box(contentAlignment = Alignment.Center, )
block and its matching closing brace so the inner Column becomes the direct
child of the parent composable; this removes a pointless layout node since Box's
contentAlignment is unused when only a single Column child is present (search
for Box(...) and the nested Column in ExportTransactionsDialog to locate the
code).
- Around line 83-86: The To-date picker currently hardcodes the minimum as
"2000-01-01" so users can pick a to-date earlier than the selected from-date;
update the toDatePickerState to react to the selected fromDate by recreating it
with the appropriate selectableDates (use
createSelectableDatesFrom(LocalDate.parse(fromDate)) or equivalent) so the
minimum equals the chosen fromDate; implement this by wrapping the
rememberDatePickerState call for toDatePickerState in key(fromDate) and
constructing selectableDates from the parsed fromDate so the picker prevents
invalid ranges (note you may need to add handling to preserve an existing valid
toDate if desired).
There was a problem hiding this comment.
Can you tell us the reason for implementing a separate date picker instead of using import androidx.compose.material3.DatePickerDialog? Please refer other modules where such an implementation is already present.
There was a problem hiding this comment.
MifosDatePickerDialog in ui directly wraps androidx.compose.material3.DatePickerDialog under the hood (you can see the import )
There was a problem hiding this comment.
I meant we could use the Material DatePickerDialog directly as done in other modules instead of introducing new core/ui components.
There was a problem hiding this comment.
Can you please explain the reason for implementing this? There are several screens where similar inputs are handled without such an implementation.
As a general rule, the codebase already contains multiple existing implementations for common patterns. It’s important to look for and reuse those wherever applicable in your current task.
There was a problem hiding this comment.
should I undo this?
There was a problem hiding this comment.
Please refer to the similar existing implementation in the codebase instead of introducing core/ui components. If the required reference implementation is not present, then we should first discuss it in the project's Slack channel before implementing it in a PR.
There was a problem hiding this comment.
This file shouldn't be a part of this PR. Please revert this change.
There was a problem hiding this comment.
mb this was pushed by mistake will undo this
…-dialog' into feature/loan-transactions-export-dialog
niyajali
left a comment
niyajali
left a comment
There was a problem hiding this comment.
@gurnoorpannu All of your pull requests have similar problems. Examine other projects, such as mifos-pay to see how the ViewModel and Screen were implemented. Update all of your pull requests to match and then request reviews again.
| LoanTransactionsScreen( | ||
| uiState = uiState, | ||
| navigateBack = navigateBack, | ||
| onRetry = { |
There was a problem hiding this comment.
don't call any methods of ViewModel directly, consider send an appropriate action
|
@coderabbitai Improve PR title based on github.com/openMF/mobile-wallet/wiki/Commit-style-guide. Give us just the title. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain
🌐 Web query: 💡 Result: The openMF Mobile Wallet project (now hosted as
The repository README links to this Wiki page under “How to Contribute” → “commit style guidelines”. [2] Sources: [1] (github.com), [2] (github.com) Citations:
🧠 Learnings used |
biplab1
changed the title
| } | ||
| val uiState by viewModel.uiState.collectAsStateWithLifecycle() | ||
| val selectedRow by viewModel.selectedRow.collectAsStateWithLifecycle() | ||
| val exportDialogState by viewModel.exportDialogState.collectAsStateWithLifecycle() |
There was a problem hiding this comment.
as we are following MVI , all 3 should be in single state
There was a problem hiding this comment.
no need of maintaining individual states
| exportDialogState = exportDialogState, | ||
| onNavigateBack = navigateBack, | ||
| onRetry = viewModel::retry, | ||
| onRowSelected = viewModel::onRowSelected, |
There was a problem hiding this comment.
craete onAction() and handle actions in viewmodel see mifos-mobile or wallet for reference
| onRetry: () -> Unit = {}, | ||
| onRowSelected: (LoanTransactionsUiState.LoanTransactionsTableData.TransactionRowData) -> Unit = {}, | ||
| onDismissBottomSheet: () -> Unit = {}, | ||
| onTransactionAction: (TransactionAction, Int) -> Unit = { _, _ -> }, |
There was a problem hiding this comment.
pass only OnAction and remove all lamdas
|
@revanthkumarJ made the changes |
| commonMain.dependencies { | ||
| api(projects.core.designsystem) | ||
| api(projects.core.common) | ||
| api(libs.kotlinx.datetime) |
There was a problem hiding this comment.
can i know why you added api(projects.core.common) if tehre are no chnages in core:ui
There was a problem hiding this comment.
oh my bad will fix this
| val snackbarHostState = remember { | ||
| SnackbarHostState() | ||
| } | ||
| val snackbarHostState = remember { SnackbarHostState() } |
There was a problem hiding this comment.
can you confirm once have we used snackbarHostState anywhere if it is not used to show any meessages there is no need to pass it
There was a problem hiding this comment.
yeah , saw that . will remove this too
|
8 participants
Fixes - Jira-#657
Before:-
After:-
Recording:-
FILE.2026-02-16.18.20.06.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