Implement New Share Account – Details Step #2530
Conversation
WalkthroughThis pull request introduces a complete Share Account feature to the application, adding network service integration, repository patterns, multi-step creation screens, and dependency injection wiring to enable clients to create and manage share accounts. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Nav as Navigation
participant Screen as CreateShareAccountScreen
participant VM as CreateShareAccountViewModel
participant Repo as ShareAccountRepository
participant API as ShareAccountService
User->>Nav: Click "Apply for Share Account"
Nav->>Nav: Pass clientId to route
Nav->>Screen: Show with clientId
Screen->>VM: Initialize with clientId
VM->>VM: Check network status
VM->>Repo: getShareTemplate(clientId)
Repo->>API: shareProductTemplate(clientId)
API-->>Repo: Flow<ShareTemplate>
Repo-->>VM: DataState.Loading
VM->>Screen: Update state (Loading)
API-->>Repo: DataState.Success(template)
VM->>Screen: Update state with productOptions
User->>Screen: Fill Details step (select product, dates, ID)
Screen->>VM: onAction(OnDetailNext)
VM->>VM: Validate, moveToNextStep()
rect rgb(200, 220, 255)
Note over Screen,VM: Proceed through Terms, Charges steps
end
User->>Screen: Confirm Preview step
Screen->>VM: onAction(Finish)
VM->>Screen: Emit ShareAccountEvent.Finish
Screen->>Nav: Navigate back (onFinish callback)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Areas requiring extra attention:
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: 3
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/clientApplyNewApplications/ClientApplyNewApplicationsScreen.kt (1)
173-177: Fix copy-paste error in subtitle resource.Line 175 incorrectly uses
client_apply_new_applications_apply_loan_accountas the subtitle for NewShareAccount. This should reference a share-specific resource to maintain consistency with other account types (NewLoanAccount usesapply_loan_account, NewSavingsAccount usesapply_savings_account, etc.).Apply this diff if the corresponding string resource exists:
data object NewShareAccount : ClientApplyNewApplicationsItem( title = Res.string.client_apply_new_applications_share_account, - subTitle = Res.string.client_apply_new_applications_apply_loan_account, + subTitle = Res.string.client_apply_new_applications_apply_share_account, icon = Res.drawable.stacked_bar_chart, )If the resource doesn't exist yet, it should be added to the string resources file.
🧹 Nitpick comments (3)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/CreateShareAccountViewModel.kt (3)
201-205: Consider usingdata objectfor consistency.Modern Kotlin (1.9+) recommends using
data objectinstead ofobjectfor sealed interface implementations. This provides better semantics and consistency with thedata class Errorvariant.Apply this diff:
interface ScreenState { - object Loading : ScreenState - object Success : ScreenState + data object Loading : ScreenState + data object Success : ScreenState data class Error(val message: String) : ScreenState }
208-220: Standardize ondata objectfor consistency.The sealed interface mixes
objectanddata objectdeclarations for parameterless actions. For consistency and better semantics in Kotlin 1.9+, usedata objectfor all parameterless sealed interface members.Apply this diff:
sealed interface ShareAccountAction { - object NextStep : ShareAccountAction + data object NextStep : ShareAccountAction data object PreviousStep : ShareAccountAction - data class OnStepChange(val index: Int) : ShareAccountAction - object NavigateBack : ShareAccountAction - object Finish : ShareAccountAction + data class OnStepChange(val index: Int) : ShareAccountAction + data object NavigateBack : ShareAccountAction + data object Finish : ShareAccountAction data class OnShareProductChange(val index: Int) : ShareAccountAction data class OnDateChange(val date: String) : ShareAccountAction data class OnOpenDatePicker(val state: Boolean) : ShareAccountAction data class OnExternalIdChange(val value: String?) : ShareAccountAction - object OnDetailNext : ShareAccountAction - object Retry : ShareAccountAction + data object OnDetailNext : ShareAccountAction + data object Retry : ShareAccountAction }
222-225: Standardize ondata objectfor consistency.Use
data objectinstead ofobjectfor sealed interface members to align with Kotlin 1.9+ best practices.Apply this diff:
sealed interface ShareAccountEvent { - object NavigateBack : ShareAccountEvent - object Finish : ShareAccountEvent + data object NavigateBack : ShareAccountEvent + data object Finish : ShareAccountEvent }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
core/data/src/commonMain/kotlin/com/mifos/core/data/di/RepositoryModule.kt(3 hunks)core/data/src/commonMain/kotlin/com/mifos/core/data/repository/ShareAccountRepository.kt(1 hunks)core/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/ShareAccountRepositoryImpl.kt(1 hunks)core/database/src/commonMain/kotlin/com/mifos/room/basemodel/APIEndPoint.kt(1 hunks)core/network/src/commonMain/kotlin/com/mifos/core/network/BaseApiManager.kt(3 hunks)core/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerShare.kt(1 hunks)core/network/src/commonMain/kotlin/com/mifos/core/network/di/DataMangerModule.kt(2 hunks)core/network/src/commonMain/kotlin/com/mifos/core/network/model/share/ProductOption.kt(1 hunks)core/network/src/commonMain/kotlin/com/mifos/core/network/model/share/ShareTemplate.kt(1 hunks)core/network/src/commonMain/kotlin/com/mifos/core/network/services/ShareAccountService.kt(1 hunks)feature/client/src/commonMain/composeResources/values/strings.xml(1 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientApplyNewApplications/ClientApplyNewApplicationRoute.kt(1 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientApplyNewApplications/ClientApplyNewApplicationsScreen.kt(2 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/CreateShareAccountRoute.kt(1 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/CreateShareAccountScreen.kt(1 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/CreateShareAccountViewModel.kt(1 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/ShareAccountScreen.kt(0 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/ShareAccountViewModel.kt(0 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/pages/DetailsPage.kt(1 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/di/ClientModule.kt(2 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/navigation/ClientNavigation.kt(3 hunks)
💤 Files with no reviewable changes (2)
- feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/ShareAccountViewModel.kt
- feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/ShareAccountScreen.kt
🧰 Additional context used
🧬 Code graph analysis (4)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/CreateShareAccountRoute.kt (1)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/CreateShareAccountScreen.kt (1)
CreateShareAccountScreen(41-64)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/pages/DetailsPage.kt (3)
core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/component/MifosTextFieldDropdown.kt (1)
MifosTextFieldDropdown(39-112)core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/component/MifosOutlinedTextField.kt (1)
MifosDatePickerTextField(363-402)core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosTwoButtonRow.kt (1)
MifosTwoButtonRow(31-91)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/CreateShareAccountScreen.kt (6)
core/ui/src/commonMain/kotlin/com/mifos/core/ui/util/EventsEffect.kt (1)
EventsEffect(28-43)feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/pages/DetailsPage.kt (1)
DetailsPage(50-152)core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosProgressIndicator.kt (1)
MifosProgressIndicator(41-68)core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosBreadCrumb.kt (1)
MifosBreadcrumbNavBar(36-107)core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosStepper.kt (1)
MifosStepper(47-139)core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/component/MifosSweetError.kt (1)
MifosSweetError(40-83)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/navigation/ClientNavigation.kt (1)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/CreateShareAccountRoute.kt (1)
createShareAccountDestination(22-30)
🔇 Additional comments (16)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientApplyNewApplications/ClientApplyNewApplicationsScreen.kt (2)
64-64: LGTM! Navigation signature aligned with other account types.The parameter change from
() -> Unitto(Int) -> Unitcorrectly matches the pattern established byonNavigateApplyLoanAccountandonNavigateApplySavingsAccount, enabling client-specific navigation.
87-87: LGTM! Invocation correctly passes clientId.The call to
onNavigateApplyShareAccount(state.clientId)properly aligns with the updated signature and follows the same pattern as NewLoanAccount and NewSavingsAccount handlers.feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientApplyNewApplications/ClientApplyNewApplicationRoute.kt (1)
27-27: LGTM! Navigation signature updated correctly.The parameter type change from
() -> Unitto(Int) -> Unitaligns with the other navigation parameters for loan and savings accounts, enabling consistent clientId propagation across the application flow.core/network/src/commonMain/kotlin/com/mifos/core/network/di/DataMangerModule.kt (1)
30-30: LGTM! DI binding follows established pattern.The
DataManagerSharesingleton registration is consistent with other data manager bindings in the module, properly resolving itsBaseApiManagerdependency viaget().Also applies to: 55-55
core/database/src/commonMain/kotlin/com/mifos/room/basemodel/APIEndPoint.kt (1)
43-43: LGTM! Endpoint constant added correctly.The
SHAREconstant follows the naming convention of other API endpoint constants and will be used by the ShareAccountService for path construction.core/network/src/commonMain/kotlin/com/mifos/core/network/BaseApiManager.kt (1)
44-44: LGTM! Service integration follows established pattern.The
shareAccountServiceis properly initialized using the Ktorfit factory method, consistent with all other service declarations in the class. The public visibility enables access fromDataManagerShare.Also applies to: 63-63, 92-92
core/data/src/commonMain/kotlin/com/mifos/core/data/repository/ShareAccountRepository.kt (1)
16-19: LGTM! Repository interface follows clean architecture principles.The
ShareAccountRepositoryinterface is well-designed with a single responsibility and uses the reactive Flow pattern withDataStatewrapping for consistent error handling across the application.feature/client/src/commonMain/kotlin/com/mifos/feature/client/di/ClientModule.kt (1)
40-40: LGTM! ViewModel registration follows module convention.The
CreateShareAccountViewModelis properly registered usingviewModelOf, consistent with all other ViewModel bindings in the ClientModule. Koin will automatically resolve its dependencies at runtime.Also applies to: 85-85
feature/client/src/commonMain/kotlin/com/mifos/feature/client/navigation/ClientNavigation.kt (1)
73-74: LGTM! Navigation refactoring is complete and consistent.The navigation updates successfully transition from the old share account flow to the new create share account flow:
- Import statements updated to reference the new destination and navigation functions
- The navigation call at line 312 now properly passes
clientIdtonavigateToCreateShareAccountRoute- Destination registration updated with the required
navControllerparameterAll changes are coordinated and follow the navigation patterns established in the codebase.
Also applies to: 312-312, 346-348
core/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerShare.kt (1)
16-22: LGTM! Data manager implementation is clean and focused.The
DataManagerShareclass follows a simple delegation pattern, passing through the Flow fromShareAccountServicewithout unnecessary intermediate processing. This keeps the data manager layer thin and delegates error handling to the repository layer whereDataStatewrapping occurs.feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/CreateShareAccountViewModel.kt (5)
32-44: LGTM! Clean ViewModel setup.The ViewModel follows proper dependency injection patterns and correctly initializes the share template loading on creation.
46-86: LGTM! Proper network and state handling.The function correctly checks network availability before making API calls and properly handles all DataState transitions with appropriate UI state updates.
88-97: LGTM! Proper boundary checking.The function correctly validates the step boundary before incrementing.
120-182: LGTM! Comprehensive action handling.All actions are properly handled with appropriate state updates and event emissions.
108-118: ****The current code is correct and intentional. Passing an empty string to
TextFieldsValidator.stringValidator("")is the proper approach because the validator is designed to handle this case: when input is blank, it returnsRes.string.error_field_empty, which is exactly what should be set. This reuses the existing validation logic rather than bypassing it. Additionally, the review's suggestion usesRes.string.field_empty, which is a different resource string than what the validator returns (error_field_empty).Likely an incorrect or invalid review comment.
feature/client/src/commonMain/composeResources/values/strings.xml (1)
535-551: LGTM! Well-organized string resources.The new string resources are properly named and organized, supporting the Share Account creation flow with clear labels for steps, form fields, and error messages. The generic
field_emptymessage is reusable across the application.
| @GET("accounts/" + APIEndPoint.SHARE + "/template") | ||
| fun shareProductTemplate( | ||
| @Query("clientId") clientId: Int, | ||
| ): Flow<ShareTemplate> |
There was a problem hiding this comment.
Expose optional productId in the share template call.
Without the productId query arg we can only retrieve the bare template (client + product list). The Fineract share template API expects us to pass the selected share product id to receive the product-specific defaults (charges, currency, etc.) that the later Terms/Charges steps rely on. Add an optional productId parameter here and plumb it through the repository/DataManager. (demo.mifos.io)
- fun shareProductTemplate(
- @Query("clientId") clientId: Int,
- ): Flow<ShareTemplate>
+ fun shareProductTemplate(
+ @Query("clientId") clientId: Int,
+ @Query("productId") productId: Int? = null,
+ ): Flow<ShareTemplate>🤖 Prompt for AI Agents
In
core/network/src/commonMain/kotlin/com/mifos/core/network/services/ShareAccountService.kt
around lines 20 to 23, the shareProductTemplate endpoint needs to accept an
optional productId query parameter so the API can return product-specific
defaults; add a nullable productId: Int? parameter annotated with
@Query("productId") to the function signature (defaulting to null) and update
all callers by plumbing this parameter through the repository and DataManager
layers (add optional productId parameters to repository/DataManager methods and
pass them to the service call) so existing behavior remains unchanged when
productId is omitted.
| CreateShareAccountScreen( | ||
| onNavigateBack = navController::popBackStack, | ||
| onFinish = {}, | ||
| navController = navController, | ||
| ) |
There was a problem hiding this comment.
Wire the finish callback to a navigation action.
Right now we pass onFinish = {} so when the flow emits ShareAccountEvent.Finish nothing happens—the user is stuck on the review screen after submitting. Hook the lambda up to navController::popBackStack (or the appropriate success destination) so the flow actually exits.
CreateShareAccountScreen(
onNavigateBack = navController::popBackStack,
- onFinish = {},
+ onFinish = { navController.popBackStack() },
navController = navController,
)🤖 Prompt for AI Agents
In
feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/CreateShareAccountRoute.kt
around lines 24 to 28, the onFinish callback is an empty lambda so
ShareAccountEvent.Finish doesn't navigate away; replace onFinish = {} with a
navigation action such as navController::popBackStack (or another success
destination) so the flow exits the review screen when finished.
| private fun moveToPreviousStep() { | ||
| val current = state.currentStep | ||
| mutableStateFlow.update { | ||
| it.copy( | ||
| currentStep = current - 1, | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
Add boundary check to prevent negative step values.
The function decrements currentStep without checking if it's already at the minimum (0). This could result in a negative step value if called when currentStep == 0.
Apply this diff to add the boundary check:
private fun moveToPreviousStep() {
val current = state.currentStep
- mutableStateFlow.update {
- it.copy(
- currentStep = current - 1,
- )
+ if (current > 0) {
+ mutableStateFlow.update {
+ it.copy(
+ currentStep = current - 1,
+ )
+ }
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private fun moveToPreviousStep() { | |
| val current = state.currentStep | |
| mutableStateFlow.update { | |
| it.copy( | |
| currentStep = current - 1, | |
| ) | |
| } | |
| } | |
| private fun moveToPreviousStep() { | |
| val current = state.currentStep | |
| if (current > 0) { | |
| mutableStateFlow.update { | |
| it.copy( | |
| currentStep = current - 1, | |
| ) | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In
feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/CreateShareAccountViewModel.kt
around lines 99 to 106, moveToPreviousStep decrements currentStep without
checking lower bound; add a guard to ensure currentStep is > 0 before updating
state (i.e., read current = state.currentStep and only call
mutableStateFlow.update to set currentStep = current - 1 when current > 0),
leaving state unchanged otherwise.
2 participants
Fixes - Jira-#543
Summary by CodeRabbit
Release Notes