Fixed login Issue using mifos/password(Login credentials)#3071
Fixed login Issue using mifos/password(Login credentials)#3071niyajali merged 31 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:
📝 WalkthroughWalkthroughUpdated login error handling to use localized Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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: 1
🤖 Fix all issues with AI agents
In
`@feature/auth/src/commonMain/kotlin/org/mifos/mobile/feature/auth/login/LoginViewModel.kt`:
- Around line 112-122: Replace the hardcoded error message in LoginViewModel
(inside the user.clients.isEmpty() branch) with a string resource: add
feature_sign_in_no_linked_client_error to your resources and pass a Res.string
reference (consistent with other messages that use Res.string.*) into
DialogState.Error instead of the literal String; if DialogState.Error currently
only accepts String, update its constructor/type to accept a StringResource/Res
or resolve the resource before calling updateState, and also remove the extra
leading whitespace/indentation in that block to match surrounding formatting.
🧹 Nitpick comments (2)
core/datastore/src/commonMain/kotlin/org/mifos/mobile/core/datastore/UserPreferencesDataSource.kt (1)
160-170: Correct fix - now aligns with the update pattern used elsewhere in the class.The addition of
_userInfo.value = UserData.DEFAULTensures the in-memory state is synchronized with persistent storage, matching the pattern used inupdateUserInfo,updateToken,updateClientId, etc. This is the key fix for the auth state inconsistency.Minor: The indentation on line 163 appears to have extra leading whitespace compared to the surrounding code.
🔧 Suggested indentation fix
suspend fun clearInfo() { withContext(dispatcher) { settings.putUserPreference(UserData.DEFAULT) - _userInfo.value = UserData.DEFAULT + _userInfo.value = UserData.DEFAULT val cleared = settings.getSettingsPreference().copy( isAuthenticated = false, )feature/auth/src/commonMain/kotlin/org/mifos/mobile/feature/auth/login/LoginViewModel.kt (1)
123-135: Simplify redundantisNotEmpty()check.Since the early return at line 121-122 guarantees
user.clientsis not empty at this point, the conditional on lines 126-130 is redundant. You can safely useuser.clients[0]directly.♻️ Suggested simplification
val userData = UserData( userId = user.userId, userName = user.username.orEmpty(), - clientId = if (user.clients.isNotEmpty()) { - user.clients[0] - } else { - user.userId - }, + clientId = user.clients[0], isAuthenticated = user.isAuthenticated, base64EncodedAuthenticationKey = user.base64EncodedAuthenticationKey.orEmpty(), officeName = user.officeName.orEmpty(), password = state.password, )
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cmp-android/src/main/AndroidManifest.xmlcore/datastore/src/commonMain/kotlin/org/mifos/mobile/core/datastore/UserPreferencesDataSource.ktfeature/auth/src/commonMain/kotlin/org/mifos/mobile/feature/auth/login/LoginViewModel.kt
🧰 Additional context used
🧬 Code graph analysis (1)
feature/auth/src/commonMain/kotlin/org/mifos/mobile/feature/auth/login/LoginViewModel.kt (10)
feature/home/src/commonMain/kotlin/org/mifos/mobile/feature/home/HomeViewModel.kt (1)
updateState(211-213)feature/auth/src/commonMain/kotlin/org/mifos/mobile/feature/auth/registration/RegistrationViewModel.kt (1)
updateState(61-63)feature/beneficiary/src/commonMain/kotlin/org/mifos/mobile/feature/beneficiary/beneficiaryDetail/BeneficiaryDetailViewModel.kt (1)
updateState(100-102)feature/accounts/src/commonMain/kotlin/org/mifos/mobile/feature/accounts/accountTransactions/TransactionViewModel.kt (1)
updateState(116-118)feature/savings-application/src/commonMain/kotlin/org/mifos/mobile/feature/savings/application/savingsApplication/SavingsApplyViewModel.kt (1)
updateState(167-169)feature/beneficiary/src/commonMain/kotlin/org/mifos/mobile/feature/beneficiary/beneficiaryApplication/BeneficiaryApplicationViewModel.kt (1)
updateState(96-98)feature/beneficiary/src/commonMain/kotlin/org/mifos/mobile/feature/beneficiary/beneficiaryList/BeneficiaryListViewModel.kt (1)
updateState(131-133)feature/qr/src/commonMain/kotlin/org/mifos/mobile/feature/qr/qr/QrCodeReaderViewModel.kt (1)
updateState(63-65)feature/qr/src/commonMain/kotlin/org/mifos/mobile/feature/qr/qrCodeDisplay/QrCodeDisplayViewModel.kt (1)
updateState(91-93)feature/qr/src/commonMain/kotlin/org/mifos/mobile/feature/qr/qrCodeImport/QrCodeImportViewModel.kt (1)
updateState(59-61)
🔇 Additional comments (1)
cmp-android/src/main/AndroidManifest.xml (1)
37-37: Good security and reliability improvement.Disabling
android:allowBackupprevents Android from restoring stale authentication credentials after app reinstall or device migration, which aligns with the PR's goal of fixing auth state inconsistencies. This is a best practice for apps handling sensitive user credentials.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Aditya002500
left a comment
Aditya002500
left a comment
There was a problem hiding this comment.
Fixed indentation
|
@Aditya002500 The ticket number seems to be incorrect. Please check. |
|
Please update the ticket number and its corresponding URL in the PR description. |
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 (4)
feature/settings/src/commonMain/kotlin/org/mifos/mobile/feature/settings/di/SettingsModule.kt (1)
27-35: Duplicate ViewModel registration:UpdatePasscodeViewModelis registered twice.
viewModelOf(::UpdatePasscodeViewModel)appears at both line 29 and line 31. Remove the duplicate registration.🐛 Proposed fix
val SettingsModule = module { viewModelOf(::SettingsViewModel) viewModelOf(::UpdatePasscodeViewModel) viewModelOf(::ChangePasswordViewModel) - viewModelOf(::UpdatePasscodeViewModel) viewModelOf(::FaqViewModel) viewModelOf(::LanguageViewModel) viewModelOf(::ChangeThemeViewModel) viewModelOf(::SelectedServicesViewModel) }core/datastore/src/commonMain/kotlin/org/mifos/mobile/core/datastore/UserPreferencesRepositoryImpl.kt (1)
139-146: Pre-existing null safety issue:clientId!!will throw NPE if null is passed.The method signature accepts
Long?but immediately force-unwraps it. Consider handling the null case gracefully or changing the parameter type to non-nullableLong.Suggested fix
override suspend fun updateClientId(clientId: Long?): DataState<Unit> { return try { - val result = preferenceManager.updateClientId(clientId!!) + val id = clientId ?: return DataState.Error(IllegalArgumentException("clientId cannot be null")) + val result = preferenceManager.updateClientId(id) DataState.Success(result) } catch (e: Exception) { DataState.Error(e) } }feature/home/src/commonMain/kotlin/org/mifos/mobile/feature/home/HomeViewModel.kt (1)
317-329: Pre-existing null safety issue: force unwrap on nullable parameter.
savingAccountListis typed asList<SavingAccount>?but is force-unwrapped with!!at line 319. While the current call site at line 257 guards this withisNotEmpty(), the function signature doesn't guarantee a non-null list.Consider making the parameter non-nullable or using safe iteration
- private fun getSavingAccountDetails(savingAccountList: List<SavingAccount>?) { + private fun getSavingAccountDetails(savingAccountList: List<SavingAccount>) { var totalAmount = 0.0 - for (savingAccount in savingAccountList!!) { + for (savingAccount in savingAccountList) { totalAmount += savingAccount.accountBalance }Or if the nullable type must be retained:
- for (savingAccount in savingAccountList!!) { + for (savingAccount in savingAccountList.orEmpty()) {core/datastore/src/commonMain/kotlin/org/mifos/mobile/core/datastore/UserPreferencesDataSource.kt (1)
160-170:clearInfo()should also reset selected services to defaults.When a user logs out, the
_selectedServicesstate flow and the persistedSELECTED_SERVICESpreference retain their values. Since selected services are user-specific (as shown by their use in HomeViewModel to filter service cards per user), the next user logging in would initially see the previous user's service selections. Reset both_selectedServicesand the stored preference toDEFAULT_SELECTED_SERVICESinclearInfo()to maintain user data separation.
🤖 Fix all issues with AI agents
In
`@core/datastore/src/commonMain/kotlin/org/mifos/mobile/core/datastore/UserPreferencesDataSource.kt`:
- Around line 239-246: getSelectedServices() currently treats an empty stored
string the same as "not configured" because
settings.getString(SELECTED_SERVICES, "") returns "" for both cases; change the
logic to distinguish absent vs explicit empty by either (A) checking presence
via settings.contains(SELECTED_SERVICES) (or using getString with a nullable
default) and returning DEFAULT_SELECTED_SERVICES only when the key is missing,
while returning emptySet() when the stored value is "" — or (B) introduce a
separate boolean flag (e.g., SELECTED_SERVICES_CONFIGURED) that you set when the
user saves selections and check in getSelectedServices(); update the code paths
that write the preference (the setter that calls settings.putString or writes
SELECTED_SERVICES_CONFIGURED) and modify getSelectedServices() accordingly so
DEFAULT_SELECTED_SERVICES is returned only for "not yet configured" and an
explicit empty string yields an empty set.
🧹 Nitpick comments (3)
feature/settings/src/commonMain/kotlin/org/mifos/mobile/feature/settings/componenets/SettingsItems.kt (1)
108-116: Verify icon choice:MifosIcons.AppRecentis also used byAppInfo.The
SelectedServicesitem usesMifosIcons.AppRecent, which is the same icon used byAppInfo(line 168). Consider using a distinct icon for better visual differentiation in the settings list.feature/settings/src/commonMain/kotlin/org/mifos/mobile/feature/settings/selectedservices/SelectedServicesViewModel.kt (1)
57-64: Use a stable snapshot for apply; verify failure handling.
CapturingpendingSelectedServicesbefore launching avoids stale reads. Also, if persistence can fail, consider surfacing that instead of navigating back.♻️ Suggested adjustment
private fun handleApplyChanges() { - viewModelScope.launch { - userPreferencesRepository.setSelectedServices(state.pendingSelectedServices) - mutableStateFlow.update { - it.copy(currentSelectedServices = state.pendingSelectedServices) - } + val pending = state.pendingSelectedServices + viewModelScope.launch { + userPreferencesRepository.setSelectedServices(pending) + mutableStateFlow.update { + it.copy(currentSelectedServices = pending) + } sendEvent(SelectedServicesEvent.NavigateBack) } }feature/settings/src/commonMain/kotlin/org/mifos/mobile/feature/settings/selectedservices/SelectedServicesScreen.kt (1)
98-129: Apply themodifierto the scaffold instead of the button.
Right now the caller’smodifieronly affects the bottom button, which is surprising for a screen-level modifier.♻️ Suggested adjustment
MifosElevatedScaffold( + modifier = modifier, onNavigateBack = { onAction(SelectedServicesAction.OnNavigateBack) }, topBarTitle = stringResource(Res.string.feature_settings_selected_services_topbar), bottomBar = { MifosButton( - modifier = modifier + modifier = Modifier .fillMaxWidth() .height(DesignToken.sizes.buttonHeight) .padding(horizontal = KptTheme.spacing.md), shape = KptTheme.shapes.medium,
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
core/common/src/commonMain/kotlin/org/mifos/mobile/core/common/Constants.ktcore/datastore/src/commonMain/kotlin/org/mifos/mobile/core/datastore/UserPreferencesDataSource.ktcore/datastore/src/commonMain/kotlin/org/mifos/mobile/core/datastore/UserPreferencesRepository.ktcore/datastore/src/commonMain/kotlin/org/mifos/mobile/core/datastore/UserPreferencesRepositoryImpl.ktfeature/home/src/commonMain/kotlin/org/mifos/mobile/feature/home/HomeViewModel.ktfeature/settings/src/commonMain/composeResources/values/strings.xmlfeature/settings/src/commonMain/kotlin/org/mifos/mobile/feature/settings/componenets/SettingsItems.ktfeature/settings/src/commonMain/kotlin/org/mifos/mobile/feature/settings/di/SettingsModule.ktfeature/settings/src/commonMain/kotlin/org/mifos/mobile/feature/settings/navigation/SettingsNavGraphRoute.ktfeature/settings/src/commonMain/kotlin/org/mifos/mobile/feature/settings/selectedservices/SelectedServicesRoute.ktfeature/settings/src/commonMain/kotlin/org/mifos/mobile/feature/settings/selectedservices/SelectedServicesScreen.ktfeature/settings/src/commonMain/kotlin/org/mifos/mobile/feature/settings/selectedservices/SelectedServicesViewModel.kt
🧰 Additional context used
📓 Path-based instructions (1)
**/{src,}/**/di/**/*.kt
📄 CodeRabbit inference engine (CLAUDE.md)
Define Koin modules in the
di/package for each module
Files:
feature/settings/src/commonMain/kotlin/org/mifos/mobile/feature/settings/di/SettingsModule.kt
🧠 Learnings (2)
📚 Learning: 2026-01-07T10:05:07.700Z
Learnt from: CR
Repo: openMF/mifos-mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-07T10:05:07.700Z
Learning: Use ViewModels with StateFlow/SharedFlow for state management in feature modules
Applied to files:
feature/settings/src/commonMain/kotlin/org/mifos/mobile/feature/settings/selectedservices/SelectedServicesViewModel.ktfeature/settings/src/commonMain/kotlin/org/mifos/mobile/feature/settings/di/SettingsModule.ktfeature/home/src/commonMain/kotlin/org/mifos/mobile/feature/home/HomeViewModel.kt
📚 Learning: 2026-01-07T10:05:07.700Z
Learnt from: CR
Repo: openMF/mifos-mobile PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-07T10:05:07.700Z
Learning: Use the `ScreenState<T>` pattern for loading/success/error states in feature modules
Applied to files:
feature/home/src/commonMain/kotlin/org/mifos/mobile/feature/home/HomeViewModel.kt
🧬 Code graph analysis (2)
feature/settings/src/commonMain/kotlin/org/mifos/mobile/feature/settings/selectedservices/SelectedServicesScreen.kt (2)
core/designsystem/src/commonMain/kotlin/org/mifos/mobile/core/designsystem/component/MifosScaffold.kt (1)
MifosElevatedScaffold(209-284)core/designsystem/src/commonMain/kotlin/org/mifos/mobile/core/designsystem/theme/Theme.kt (1)
MifosMobileTheme(267-291)
feature/settings/src/commonMain/kotlin/org/mifos/mobile/feature/settings/navigation/SettingsNavGraphRoute.kt (1)
feature/settings/src/commonMain/kotlin/org/mifos/mobile/feature/settings/selectedservices/SelectedServicesRoute.kt (1)
selectedServicesDestination(22-30)
🔇 Additional comments (23)
core/common/src/commonMain/kotlin/org/mifos/mobile/core/common/Constants.kt (1)
111-111: LGTM!The constant follows the existing naming conventions and is appropriately placed in the settings constants section.
feature/settings/src/commonMain/kotlin/org/mifos/mobile/feature/settings/componenets/SettingsItems.kt (2)
36-37: LGTM!Imports follow the established pattern for other settings items.
192-192: LGTM!The new item is properly added to the settings list in a logical position.
feature/settings/src/commonMain/composeResources/values/strings.xml (1)
71-85: LGTM!String resources follow the established naming conventions and are well-organized. The "A/c" abbreviation for "Account" is used consistently across related entries.
feature/settings/src/commonMain/kotlin/org/mifos/mobile/feature/settings/navigation/SettingsNavGraphRoute.kt (2)
25-25: LGTM!Import follows the established pattern for other destination imports.
90-92: LGTM!The
selectedServicesDestinationis wired consistently with other destinations in the settings graph, usingpopBackStackfor the back navigation action.core/datastore/src/commonMain/kotlin/org/mifos/mobile/core/datastore/UserPreferencesRepository.kt (1)
78-80: LGTM!The new interface members for observing and setting selected services are well-defined and consistent with the existing API patterns in this repository interface.
feature/settings/src/commonMain/kotlin/org/mifos/mobile/feature/settings/selectedservices/SelectedServicesRoute.kt (1)
18-30: LGTM!The navigation helpers follow the established pattern for settings routes. Using
composableWithPushTransitionsensures consistent transition animations, and the internal visibility is appropriate for feature-module navigation.core/datastore/src/commonMain/kotlin/org/mifos/mobile/core/datastore/UserPreferencesRepositoryImpl.kt (1)
190-195: LGTM!The implementation correctly delegates to
preferenceManagerfor both observing and setting selected services, following the established delegation pattern used throughout this repository.core/datastore/src/commonMain/kotlin/org/mifos/mobile/core/datastore/UserPreferencesDataSource.kt (2)
160-170: Critical fix: Resetting in-memory state after clearing persistent storage.This change correctly addresses the root cause of MM-352. Previously, only the persistent storage was cleared but the in-memory
_userInfostate retained stale values, causing authentication checks to fail and show "unexpected error occurred".
265-276: LGTM on the default services configuration.The default set of services is well-defined and covers the expected feature set. Using a companion object constant makes it reusable and testable.
feature/home/src/commonMain/kotlin/org/mifos/mobile/feature/home/HomeViewModel.kt (1)
66-77: LGTM!The observer correctly:
- Uses
distinctUntilChanged()to prevent redundant state updates- Filters from the original
serviceCardslist (not the current state) ensuring consistent behavior- Converts to
PersistentListfor Compose immutability requirementsBased on learnings, this follows the StateFlow/SharedFlow pattern recommended for ViewModels.
feature/settings/src/commonMain/kotlin/org/mifos/mobile/feature/settings/selectedservices/SelectedServicesViewModel.kt (6)
22-35: Clean initial state + observation wiring.
Initialization seeds defaults and streams repository updates into state actions clearly.
37-43: Action routing is straightforward.
The handler cleanly dispatches all action types without gaps.
46-55: Toggle logic is clear and immutable.
Set updates are concise and safe.
67-73: Load handler keeps current/pending in sync.
This makes state hydration predictable.
77-83: State model andhasChangesderivation look good.
Clear and minimal.
85-96: Event/action modeling is tidy.
Sealed interfaces keep the action space explicit.feature/settings/src/commonMain/kotlin/org/mifos/mobile/feature/settings/selectedservices/SelectedServicesScreen.kt (5)
57-73: Service catalog structure is clear.
The model and list are straightforward and readable.
75-96: Screen wiring to state + events looks solid.
The event bridge is clean and concise.
131-149: List rendering is straightforward.
The list composition reads cleanly.
151-179: Checkbox row composition looks good.
Layout and styling choices are consistent with the design system.
182-193: Preview mirrors default state nicely.
Helpful for quick UI checks.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
@Aditya002500 Is the screenshot after the changes in this PR? |
No sir but, I have updated now |
|
@Aditya002500 Can you please upload the screen recording before and after flow? |
|
Looks good to me |
|
@Aditya002500 Please fix the spotless issue by running |
|
@Aditya002500 Always keep the PR description updated with the screen recording after the recent changes. |
Sir as of now nothing is changed I just reverted some changes to original state, so everything is same. |
|
@Aditya002500 Please resolve merge conflicts. |
|
@Aditya002500 Also, please check if this PR is still required after the recent changes to |
|
Please fix the static checks issue. |
|
Also, please update a recent screen recording after the resolution of merge conflicts to ensure there is no regressions. |
….xml Co-authored-by: Biplab Dutta <biplabdutta27@gmail.com>
5 participants
Fixes - MM-352
Shows "unexpected error occurred" because client data fetches fail from the server
Both storage AND in-memory state are cleared immediately, so auth check fails correctly
Login behavior
BeforeFix.mp4
|
updated-credentials.mp4
| |
Changes in -
cmp-android\src\main\AndroidManifest.xml
core\datastore\src\commonMain\kotlin\org\mifos\mobile\core\datastore\UserPreferencesDataSource.kt
feature\auth\src\commonMain\kotlin\org\mifos\mobile\feature\auth\login\LogInNavigation.kt
values/string.xml
Summary by CodeRabbit