Order Savings Accounts by Active State Etc #2986
Conversation
WalkthroughReplaced LoanStatus with SavingStatus in savings-account UI and ViewModel; added deterministic Changes
Sequence Diagram(s)sequenceDiagram
participant Repo as Repository
participant VM as SavingsAccountViewModel
participant State as SavingsAccountState
participant Screen as SavingsAccountScreen
participant Renderer as LazyColumn / MifosAccountCard
rect #FFF8E1
Repo->>VM: fetch accounts
VM->>VM: filter results\nsortAccountsByStatus(statusOrder) -> sortedAccounts
VM->>State: emit updated state (savingsAccount = sortedAccounts)
end
rect #E8F5E9
State->>Screen: state with sortedAccounts
Screen->>Screen: accounts = state.savingsAccount.orEmpty()
Screen->>Screen: compute accountStatus per account\n(ACTIVE -> formatted balance\nSUBMIT_AND_PENDING_APPROVAL -> pending string\nelse -> status value)
end
rect #E3F2FD
Screen->>Renderer: render accounts\nchoose color by SavingStatus (ACTIVE, SUBMIT_AND_PENDING_APPROVAL, INACTIVE, else)
Renderer->>Renderer: display MifosAccountCard entries
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
feature/savings-account/src/commonMain/kotlin/org/mifos/mobile/feature/savingsaccount/savingsAccount/SavingsAccountScreen.kt(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
feature/savings-account/src/commonMain/kotlin/org/mifos/mobile/feature/savingsaccount/savingsAccount/SavingsAccountScreen.kt (1)
core/ui/src/commonMain/kotlin/org/mifos/mobile/core/ui/component/MifosAccountCard.kt (1)
MifosAccountCard(41-113)
🔇 Additional comments (3)
feature/savings-account/src/commonMain/kotlin/org/mifos/mobile/feature/savingsaccount/savingsAccount/SavingsAccountScreen.kt (3)
55-55: LGTM! Correct model import.Replacing
LoanStatuswithSavingStatusproperly aligns the savings account screen with its domain model.
279-286: Verify color mapping covers all status values.The
statusOrderlist includesSavingStatus.CLOSED(line 261), but the color mapping only handlesACTIVE,SUBMIT_AND_PENDING_APPROVAL, andINACTIVE. Accounts withCLOSEDstatus will use the defaultonSurfacecolor via theelsebranch.Please confirm this is the intended behavior.
278-278: LGTM! Sorted list correctly applied.Using
sortedAccountshere correctly applies the canonical status ordering defined above, fulfilling the PR objective.
...onMain/kotlin/org/mifos/mobile/feature/savingsaccount/savingsAccount/SavingsAccountScreen.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
feature/savings-account/src/commonMain/kotlin/org/mifos/mobile/feature/savingsaccount/savingsAccount/SavingsAccountScreen.kt (1)
258-270: Unknown statuses sort first; add safe fallback and optional tie-breaker.
indexOf returns -1 for unknown statuses, pushing them to the top. Sort them last; optionally add a secondary key for deterministic order within the same status.- val sortedAccounts = remember(state.savingsAccount) { - state.savingsAccount.orEmpty().sortedWith( - compareBy { statusOrder.indexOf(it.status?.value) }, - ) - } + val sortedAccounts = remember(state.savingsAccount) { + state.savingsAccount.orEmpty().sortedWith( + compareBy( + { account -> + val idx = statusOrder.indexOf(account.status?.value) + if (idx == -1) Int.MAX_VALUE else idx + }, + // optional: stable, human-friendly tie-breaker + { it.accountNo ?: "" }, + ), + ) + }Optional: extract statusOrder to a shared constant (e.g., core) to avoid duplication across screens.
🧹 Nitpick comments (1)
feature/savings-account/src/commonMain/kotlin/org/mifos/mobile/feature/savingsaccount/savingsAccount/SavingsAccountScreen.kt (1)
281-286: Add explicit color for CLOSED; centralize mapping.
CLOSED is in statusOrder but not colored; it falls to onSurface. Consider an explicit subdued color (e.g., onSurfaceVariant) and keep this logic in a small helper to reuse across screens.- val color = when (account.status?.value) { + val color = when (account.status?.value) { SavingStatus.ACTIVE.status -> AppColors.customEnable SavingStatus.SUBMIT_AND_PENDING_APPROVAL.status -> AppColors.customYellow - SavingStatus.INACTIVE.status -> MaterialTheme.colorScheme.error - - else -> MaterialTheme.colorScheme.onSurface + SavingStatus.CLOSED.status -> MaterialTheme.colorScheme.onSurfaceVariant + else -> MaterialTheme.colorScheme.onSurface }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
feature/savings-account/src/commonMain/kotlin/org/mifos/mobile/feature/savingsaccount/savingsAccount/SavingsAccountScreen.kt(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
feature/savings-account/src/commonMain/kotlin/org/mifos/mobile/feature/savingsaccount/savingsAccount/SavingsAccountScreen.kt (1)
core/ui/src/commonMain/kotlin/org/mifos/mobile/core/ui/component/MifosAccountCard.kt (1)
MifosAccountCard(41-113)
🔇 Additional comments (4)
feature/savings-account/src/commonMain/kotlin/org/mifos/mobile/feature/savingsaccount/savingsAccount/SavingsAccountScreen.kt (4)
43-43: Good: moved pending label to a string resource.
Using a resource instead of a hard-coded string is correct for i18n.
56-56: Correct enum usage.
Importing and using SavingStatus aligns the UI with the savings domain.
279-279: LGTM: list now renders sortedAccounts.
Keeps UI consistent with the new ordering.
306-306: LGTM: passes computed accountStatus into MifosAccountCard.
Hook-up looks correct.
...onMain/kotlin/org/mifos/mobile/feature/savingsaccount/savingsAccount/SavingsAccountScreen.kt
Show resolved
Hide resolved
...onMain/kotlin/org/mifos/mobile/feature/savingsaccount/savingsAccount/SavingsAccountScreen.kt
Outdated
Show resolved
Hide resolved
|
@biplab1 I noticed a couple of UI/UX problems with the video that was uploaded. The text "Hello jean," needs to be corrected to "Hello, Jean" and the services icon box is not aligned correctly (take a look at the beneficiary and FAQ sections for the icon box). Could you please open a ticket for these issues and assign it to someone who is available? |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
feature/savings-account/src/commonMain/kotlin/org/mifos/mobile/feature/savingsaccount/savingsAccount/SavingsAccountScreen.kt (1)
252-264: Localize INACTIVE and CLOSED status labels.As noted in previous reviews, only
SUBMIT_AND_PENDING_APPROVALis localized.CLOSEDandINACTIVEaccounts display raw backend values instead of translated strings.Add localized string keys (e.g.,
feature_savings_status_closed,feature_savings_status_inactive) to all language files, then update the logic:val accountStatus = if (account.status?.active == true) { CurrencyFormatter.format( account.accountBalance, account.currency?.code, account.currency?.decimalPlaces, ) } else { - if (account.status?.value == SavingStatus.SUBMIT_AND_PENDING_APPROVAL.status) { - stringResource(Res.string.feature_savings_filter_pending_account) - } else { - account.status?.value ?: "" - } + when (account.status?.value) { + SavingStatus.SUBMIT_AND_PENDING_APPROVAL.status -> + stringResource(Res.string.feature_savings_filter_pending_account) + SavingStatus.CLOSED.status -> + stringResource(Res.string.feature_savings_status_closed) + SavingStatus.INACTIVE.status -> + stringResource(Res.string.feature_savings_status_inactive) + else -> account.status?.value ?: "" + } }
🧹 Nitpick comments (1)
feature/savings-account/src/commonMain/kotlin/org/mifos/mobile/feature/savingsaccount/savingsAccount/SavingsAccountViewmodel.kt (1)
318-323: Consider moving statusOrder to a companion object.
statusOrderis immutable and identical across all state instances. Moving it to a companion object would avoid recreating the list unnecessarily.Apply this diff to move statusOrder to a companion object:
+ companion object { + private val STATUS_ORDER = listOf( + SavingStatus.ACTIVE.status, + SavingStatus.SUBMIT_AND_PENDING_APPROVAL.status, + SavingStatus.CLOSED.status, + SavingStatus.INACTIVE.status, + ) + } + - /** Order of statuses for consistent sorting */ - val statusOrder: List<String> = listOf( - SavingStatus.ACTIVE.status, - SavingStatus.SUBMIT_AND_PENDING_APPROVAL.status, - SavingStatus.CLOSED.status, - SavingStatus.INACTIVE.status, - ),Then update line 249 to use
STATUS_ORDERinstead ofstate.statusOrder:- return accounts.sortedWith(compareBy { state.statusOrder.indexOf(it.status?.value) }) + return accounts.sortedWith(compareBy { STATUS_ORDER.indexOf(it.status?.value) })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
feature/savings-account/src/commonMain/kotlin/org/mifos/mobile/feature/savingsaccount/savingsAccount/SavingsAccountScreen.kt(5 hunks)feature/savings-account/src/commonMain/kotlin/org/mifos/mobile/feature/savingsaccount/savingsAccount/SavingsAccountViewmodel.kt(12 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
feature/savings-account/src/commonMain/kotlin/org/mifos/mobile/feature/savingsaccount/savingsAccount/SavingsAccountScreen.kt (2)
feature/savings-application/src/commonMain/kotlin/org/mifos/mobile/feature/savings/application/savingsApplication/SavingsApplyScreen.kt (1)
SavingsAccountDialog(90-115)core/ui/src/commonMain/kotlin/org/mifos/mobile/core/ui/component/MifosAccountCard.kt (1)
MifosAccountCard(41-113)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: PR Checks KMP / Static Analysis Check
🔇 Additional comments (1)
feature/savings-account/src/commonMain/kotlin/org/mifos/mobile/feature/savingsaccount/savingsAccount/SavingsAccountScreen.kt (1)
103-103: LGTM: Clean lambda refactoring.The simplified lambda syntax with
rememberproperly memoizes the action handler and follows idiomatic Compose patterns.Also applies to: 108-108
...onMain/kotlin/org/mifos/mobile/feature/savingsaccount/savingsAccount/SavingsAccountScreen.kt
Show resolved
Hide resolved
...ain/kotlin/org/mifos/mobile/feature/savingsaccount/savingsAccount/SavingsAccountViewmodel.kt
Show resolved
Hide resolved
@WizCoderr Have you addressed all the issues mentioned above? If you have, then please upload a new video. Otherwise, let me know I will go ahead and create new tickets. |
|
My laptop is not working properly and also i got my placement paper on 31 so I will be updating all this before Monday |
|
@coderabbitai configuration |
CodeRabbit configurationlanguage: en-US
tone_instructions: ''
early_access: false
enable_free_tier: true
reviews:
profile: chill
request_changes_workflow: false
high_level_summary: true
high_level_summary_placeholder: '@coderabbitai summary'
high_level_summary_in_walkthrough: false
auto_title_placeholder: '@coderabbitai'
auto_title_instructions: ''
review_status: true
commit_status: true
fail_commit_status: false
collapse_walkthrough: false
changed_files_summary: true
sequence_diagrams: true
estimate_code_review_effort: true
assess_linked_issues: true
related_issues: true
related_prs: true
suggested_labels: true
labeling_instructions: []
auto_apply_labels: false
suggested_reviewers: true
auto_assign_reviewers: false
in_progress_fortune: true
poem: true
path_filters: []
path_instructions: []
abort_on_close: true
disable_cache: false
auto_review:
enabled: true
auto_incremental_review: true
ignore_title_keywords: []
labels: []
drafts: false
base_branches: []
ignore_usernames: []
finishing_touches:
docstrings:
enabled: true
unit_tests:
enabled: true
pre_merge_checks:
docstrings:
mode: warning
threshold: 80
title:
mode: warning
requirements: ''
description:
mode: warning
issue_assessment:
mode: warning
custom_checks: []
tools:
ast-grep:
rule_dirs: []
util_dirs: []
essential_rules: true
packages: []
shellcheck:
enabled: true
ruff:
enabled: true
markdownlint:
enabled: true
github-checks:
enabled: true
timeout_ms: 90000
languagetool:
enabled: true
enabled_rules: []
disabled_rules: []
enabled_categories: []
disabled_categories: []
enabled_only: false
level: default
biome:
enabled: true
hadolint:
enabled: true
swiftlint:
enabled: true
phpstan:
enabled: true
level: default
phpmd:
enabled: true
phpcs:
enabled: true
golangci-lint:
enabled: true
yamllint:
enabled: true
gitleaks:
enabled: true
checkov:
enabled: true
detekt:
enabled: true
eslint:
enabled: true
flake8:
enabled: true
rubocop:
enabled: true
buf:
enabled: true
regal:
enabled: true
actionlint:
enabled: true
pmd:
enabled: true
clang:
enabled: true
cppcheck:
enabled: true
semgrep:
enabled: true
circleci:
enabled: true
clippy:
enabled: true
sqlfluff:
enabled: true
prismaLint:
enabled: true
pylint:
enabled: true
oxc:
enabled: true
shopifyThemeCheck:
enabled: true
luacheck:
enabled: true
brakeman:
enabled: true
dotenvLint:
enabled: true
htmlhint:
enabled: true
checkmake:
enabled: true
osvScanner:
enabled: true
chat:
art: true
auto_reply: true
integrations:
jira:
usage: auto
linear:
usage: auto
knowledge_base:
opt_out: false
web_search:
enabled: true
code_guidelines:
enabled: true
filePatterns: []
learnings:
scope: auto
issues:
scope: auto
jira:
usage: auto
project_keys: []
linear:
usage: auto
team_keys: []
pull_requests:
scope: auto
mcp:
usage: auto
disabled_servers: []
code_generation:
docstrings:
language: en-US
path_instructions: []
unit_tests:
path_instructions: []
|
|
Screen_recording_20251030_191604.webm |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
feature/savings-account/src/commonMain/kotlin/org/mifos/mobile/feature/savingsaccount/savingsAccount/SavingsAccountViewmodel.kt (1)
290-292: Handle unknown account statuses in sort order.If
account.status?.valueis not instatusOrder,indexOfreturns-1, causing those accounts to sort first—ahead of evenACTIVEaccounts. This produces unexpected ordering.Apply this diff to sort unknown statuses last:
private fun sortAccountsByStatus(accounts: List<SavingAccount>): List<SavingAccount> { - return accounts.sortedWith(compareBy { state.statusOrder.indexOf(it.status?.value) }) + return accounts.sortedWith(compareBy { + val index = state.statusOrder.indexOf(it.status?.value) + if (index == -1) Int.MAX_VALUE else index + }) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
feature/savings-account/src/commonMain/kotlin/org/mifos/mobile/feature/savingsaccount/savingsAccount/SavingsAccountViewmodel.kt(9 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
feature/savings-account/src/commonMain/kotlin/org/mifos/mobile/feature/savingsaccount/savingsAccount/SavingsAccountViewmodel.kt (1)
feature/savings-account/src/commonMain/kotlin/org/mifos/mobile/feature/savingsaccount/savingsAccountDetails/SavingsAccountDetailsViewModel.kt (1)
updateState(130-132)
🔇 Additional comments (4)
feature/savings-account/src/commonMain/kotlin/org/mifos/mobile/feature/savingsaccount/savingsAccount/SavingsAccountViewmodel.kt (4)
26-26: LGTM!The SavingStatus import is correctly added and used in the statusOrder definition.
105-107: Good refactor for consistency.The
updateStatehelper follows the same pattern as inSavingsAccountDetailsViewModel.kt, improving code maintainability.
229-247: Sorted accounts integrated correctly.The sorted accounts are correctly applied after filtering and consistently used throughout the state updates. The implementation depends on fixing the sorting function at lines 290-292 (see existing review comment).
358-365: statusOrder includes all possible SavingStatus values—no changes needed.Verification confirms the
statusOrderlist contains all four enum values defined inSavingStatus.kt(ACTIVE, SUBMIT_AND_PENDING_APPROVAL, CLOSED, INACTIVE). The list is complete and correct.
| * A helper function to update the mutable state flow. | ||
| * | ||
| * @param update A lambda function that takes the current state and returns a new state. |
There was a problem hiding this comment.
Fix incorrect documentation.
The KDoc describes updateState but is placed above the retry() function. This appears to be a copy-paste error from lines 101-104.
Apply this diff to correct the documentation:
- /**
- * A helper function to update the mutable state flow.
- *
- * @param update A lambda function that takes the current state and returns a new state.
- */
+ /**
+ * Retries loading accounts based on network status.
+ * Shows network error if offline, otherwise reloads accounts.
+ */
private fun retry() {🤖 Prompt for AI Agents
In
feature/savings-account/src/commonMain/kotlin/org/mifos/mobile/feature/savingsaccount/savingsAccount/SavingsAccountViewmodel.kt
around lines 141-143, the KDoc that documents updateState was mistakenly left
above the retry() function; move that KDoc block to directly above the
updateState declaration (where lines ~101-104 originally referenced) and replace
the KDoc above retry() with a short, accurate comment describing retry()’s
purpose (e.g., triggers a retry of the last operation or refreshes state).
Ensure the moved KDoc matches the updateState signature and the new retry() KDoc
matches retry() behavior.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
feature/savings-account/src/commonMain/kotlin/org/mifos/mobile/feature/savingsaccount/savingsAccount/SavingsAccountViewmodel.kt (2)
288-290: Unknown statuses sort before ACTIVE.
state.statusOrder.indexOfreturns-1for any status not in the list, so those entries sort ahead ofACTIVE. That regresses the ordering whenever new statuses appear from the backend. Map missing values to the tail of the list before sorting.- return accounts.sortedWith(compareBy { state.statusOrder.indexOf(it.status?.value) }) + return accounts.sortedWith(compareBy { + val index = state.statusOrder.indexOf(it.status?.value) + if (index >= 0) index else state.statusOrder.size + })
141-144: Fix the retry() KDoc.The block still describes
updateState, which is misleading. Replace it with documentation that reflectsretry()'s behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
feature/savings-account/src/commonMain/kotlin/org/mifos/mobile/feature/savingsaccount/savingsAccount/SavingsAccountViewmodel.kt(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
feature/savings-account/src/commonMain/kotlin/org/mifos/mobile/feature/savingsaccount/savingsAccount/SavingsAccountViewmodel.kt (2)
feature/loan-account/src/commonMain/kotlin/org/mifos/mobile/feature/loanaccount/loanAccount/LoanAccountViewModel.kt (1)
updateState(105-107)feature/savings-account/src/commonMain/kotlin/org/mifos/mobile/feature/savingsaccount/savingsAccountDetails/SavingsAccountDetailsViewModel.kt (1)
updateState(130-132)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: PR Checks KMP / Static Analysis Check
...ain/kotlin/org/mifos/mobile/feature/savingsaccount/savingsAccount/SavingsAccountViewmodel.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
feature/savings-account/src/commonMain/kotlin/org/mifos/mobile/feature/savingsaccount/savingsAccount/SavingsAccountViewmodel.kt (2)
141-143: The incorrect documentation is still present.This KDoc still describes
updateStatebut is placed aboveretry(). This was flagged in the previous review but not fixed.Apply this diff to correct the documentation:
- /** - * A helper function to update the mutable state flow. - * - * @param update A lambda function that takes the current state and returns a new state. - */ + /** + * Retries loading accounts based on network status. + * Shows network error if offline, otherwise reloads accounts. + */ private fun retry() {
289-291: The sorting issue with unknown statuses is still present.If
account.status?.valueis not instatusOrderor is null,indexOfreturns-1, causing those accounts to sort first—ahead of evenACTIVEaccounts. This was flagged in the previous review but not fixed.Apply this diff to sort unknown statuses last:
private fun sortAccountsByStatus(accounts: List<SavingAccount>): List<SavingAccount> { - return accounts.sortedWith(compareBy { state.statusOrder.indexOf(it.status?.value) }) + return accounts.sortedWith(compareBy { + val index = state.statusOrder.indexOf(it.status?.value) + if (index == -1) Int.MAX_VALUE else index + }) }
🧹 Nitpick comments (1)
feature/savings-account/src/commonMain/kotlin/org/mifos/mobile/feature/savingsaccount/savingsAccount/SavingsAccountViewmodel.kt (1)
232-233: Consider usingstate.decimalsas the final fallback.The decimals fallback now includes
allSavings, which is good. However, the final fallback is still a hardcoded2instead ofstate.decimals. The previous review suggested preserving the existing state value to maintain precision across state updates.Apply this diff:
- decimals = sortedAccounts.firstOrNull()?.currency?.decimalPlaces - ?: allSavings.firstOrNull()?.currency?.decimalPlaces ?: 2, + decimals = sortedAccounts.firstOrNull()?.currency?.decimalPlaces + ?: allSavings.firstOrNull()?.currency?.decimalPlaces ?: state.decimals,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
feature/savings-account/src/commonMain/kotlin/org/mifos/mobile/feature/savingsaccount/savingsAccount/SavingsAccountViewmodel.kt(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
feature/savings-account/src/commonMain/kotlin/org/mifos/mobile/feature/savingsaccount/savingsAccount/SavingsAccountViewmodel.kt (1)
feature/savings-account/src/commonMain/kotlin/org/mifos/mobile/feature/savingsaccount/savingsAccountDetails/SavingsAccountDetailsViewModel.kt (1)
updateState(130-132)
🔇 Additional comments (1)
feature/savings-account/src/commonMain/kotlin/org/mifos/mobile/feature/savingsaccount/savingsAccount/SavingsAccountViewmodel.kt (1)
358-364: The status sort order looks reasonable.The defined order (ACTIVE → SUBMIT_AND_PENDING_APPROVAL → CLOSED → INACTIVE) makes sense for displaying accounts with active ones first.
4 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.
Screen_recording_20251028_185703.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
Bug Fixes
New Features
Refactor