Implement Share Account Preview Screen#2549
Implement Share Account Preview Screen#2549therajanmaurya merged 1 commit intoopenMF:developmentfrom
Conversation
WalkthroughThis PR adds share account creation functionality across the data, network, and UI layers. It introduces a new ShareAccountPayload model, network endpoint, repository method, and UI form with validation and success feedback. Replaces CreatedCharges with ChargeItem and refactors PreviewPage to display submission details before account creation. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–25 minutes
Possibly related PRs
Suggested reviewers
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: 4
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/createShareAccount/CreateShareAccountViewModel.kt (1)
482-489: Bug:EditChargeuses wrong index forchargeIdlookup.The
EditChargehandler retrieves the charge ID fromchargeOptions[action.index]instead of using the selectedchooseChargeIndex. Theaction.indexrepresents the position inaddedCharges, notchargeOptions:is CreateShareAccountAction.EditCharge -> { val createdData = ChargeItem( - chargeId = state.chargeOptions[action.index].id, + chargeId = state.chargeOptions[state.chooseChargeIndex!!].id, amount = state.chargeAmount.toDoubleOrNull(), )Compare with
AddChargeToListat line 468 which correctly usesstate.chooseChargeIndex.
🧹 Nitpick comments (9)
core/data/src/commonMain/kotlin/com/mifos/core/data/repository/ShareAccountRepository.kt (1)
20-21: Inconsistent function signature pattern.The
createShareAccountfunction is markedsuspendwhile also returning aFlow. This differs fromgetShareTemplateon line 20, which only returnsFlowwithoutsuspend. SinceFlowis already cold and provides async behavior, thesuspendmodifier is typically unnecessary here.Consider aligning with the existing pattern:
- suspend fun createShareAccount(shareAccountPayload: ShareAccountPayload): Flow<DataState<GenericResponse>> + fun createShareAccount(shareAccountPayload: ShareAccountPayload): Flow<DataState<GenericResponse>>This would also require updating the implementation to not be
suspend.core/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/ShareAccountRepositoryImpl.kt (1)
30-36: Thesuspendmodifier is unnecessary here.Since the actual suspend call (
dataManagerShare.createShareAccount) occurs inside theflow { }builder, the outer function doesn't need to besuspend. The flow collector will handle the suspension.- override suspend fun createShareAccount( + override fun createShareAccount( shareAccountPayload: ShareAccountPayload, ): Flow<DataState<GenericResponse>> { return flow { emit(dataManagerShare.createShareAccount(shareAccountPayload)) }.asDataStateFlow() }This aligns with the
getShareTemplatefunction pattern in this same class.core/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerShare.kt (2)
38-40: Avoid creating a newJsoninstance on every call.Creating a
Jsoninstance with configuration on each invocation is inefficient. Consider extracting this to a companion object or class-level property.class DataManagerShare( private val baseApiManager: BaseApiManager, ) { + private val json = Json { ignoreUnknownKeys = true } fun getShareTemplate(clientId: Int, productId: Int?): Flow<ShareTemplate> = baseApiManager.shareAccountService.shareProductTemplate(clientId, productId) suspend fun createShareAccount(shareAccountPayload: ShareAccountPayload): GenericResponse { val response = baseApiManager.shareAccountService.createShareAccount(shareAccountPayload) return if (!response.status.isSuccess()) { val errorMsg = extractErrorMessage(response) throw IllegalStateException(errorMsg) } else { - val json = Json { ignoreUnknownKeys = true } - json.decodeFromString<GenericResponse>(response.bodyAsText()) } } }
19-19: Unused import.The import
io.ktor.serialization.kotlinx.json.jsonappears unused in this file. The JSON parsing useskotlinx.serialization.json.Json(line 21) instead.-import io.ktor.serialization.kotlinx.json.jsonfeature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/pages/PreviewPage.kt (2)
116-116: Hardcoded strings "Yes"/"No" should use string resources for internationalization.Using hardcoded English strings bypasses the localization system.
Consider adding string resources for "Yes" and "No" and using
stringResource()here:- Res.string.feature_share_account_terms_allow_dividends to if (state.isDividendAllowed) "Yes" else "No", + Res.string.feature_share_account_terms_allow_dividends to stringResource(if (state.isDividendAllowed) Res.string.common_yes else Res.string.common_no),You'll need to add corresponding string resources if they don't already exist.
60-60: Consider using design tokens for spacing consistency.The hardcoded
20.dpdiffers from the pattern used elsewhere (e.g.,DesignToken.padding.largeon lines 56 and 100).- verticalArrangement = Arrangement.spacedBy(20.dp), + verticalArrangement = Arrangement.spacedBy(DesignToken.padding.large),Or use the appropriate token that matches 20.dp if it exists.
feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/CreateShareAccountScreen.kt (2)
121-134: Improve debug log with meaningful context.The log statement
Logger.d("SuccessResponseStatus")provides no useful diagnostic information. Consider including the actual status or message for better debugging:- Logger.d("SuccessResponseStatus") + Logger.d("SuccessResponseStatus: success=${state.dialogState.successStatus}, msg=${state.dialogState.msg}")
75-76: Consider using the existingEventsEffectpattern for consistency.The codebase has an
EventsEffectutility (referenced incore/ui/src/commonMain/kotlin/com/mifos/core/ui/util/EventsEffect.kt) that provides lifecycle-aware event handling. The current approach usesLaunchedEffectwith a random key (launchEffectKey) to trigger side effects fromDialogState.While this works, using a one-shot event flow pattern with
EventsEffect(similar to howNavigateBackandFinishevents are handled in lines 77-82) would be more consistent with the existing architecture and avoid coupling UI effects to dialog state.Also applies to: 121-134
feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/CreateShareAccountViewModel.kt (1)
467-470: Consider validating charge amount before adding.
state.chargeAmount.toDoubleOrNull()returnsnullfor invalid input, which would result in aChargeItemwith a nullamountbeing added to the list. Consider validating the amount before allowing the charge to be added:is CreateShareAccountAction.AddChargeToList -> { + val amount = state.chargeAmount.toDoubleOrNull() + if (amount == null || state.chooseChargeIndex == null) { + // Optionally show validation error + return + } val createdData = ChargeItem( - chargeId = state.chargeOptions[state.chooseChargeIndex!!].id, - amount = state.chargeAmount.toDoubleOrNull(), + chargeId = state.chargeOptions[state.chooseChargeIndex].id, + amount = amount, )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
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(2 hunks)core/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerShare.kt(1 hunks)core/network/src/commonMain/kotlin/com/mifos/core/network/model/share/ShareAccountPayload.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/createShareAccount/CreateShareAccountScreen.kt(6 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/CreateShareAccountViewModel.kt(8 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/createShareAccount/pages/PreviewPage.kt(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/pages/DetailsPage.kt (1)
core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/component/MifosOutlinedTextField.kt (1)
MifosDatePickerTextField(363-413)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/CreateShareAccountViewModel.kt (3)
core/data/src/commonMain/kotlin/com/mifos/core/data/repository/ShareAccountRepository.kt (1)
createShareAccount(21-21)core/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/ShareAccountRepositoryImpl.kt (1)
createShareAccount(30-36)core/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerShare.kt (1)
createShareAccount(30-42)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/pages/PreviewPage.kt (3)
core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosRowWithTextAndButton.kt (1)
MifosRowWithTextAndButton(35-95)core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosTwoButtonRow.kt (1)
MifosTwoButtonRow(31-91)core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosListingComponent.kt (1)
MifosDefaultListingComponentFromStringResources(244-267)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/CreateShareAccountScreen.kt (1)
core/ui/src/commonMain/kotlin/com/mifos/core/ui/util/EventsEffect.kt (1)
EventsEffect(28-43)
🔇 Additional comments (6)
feature/client/src/commonMain/composeResources/values/strings.xml (1)
554-558: LGTM!The new string resources for the submit button and success message are appropriately added. Moving the required field indicator (
*) from the resource to the code (in DetailsPage.kt) provides flexibility for conditional display.feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/pages/DetailsPage.kt (1)
120-125: LGTM!The approach of appending the required field indicator (
*) in code provides flexibility. The implementation is consistent between both required fields.core/network/src/commonMain/kotlin/com/mifos/core/network/services/ShareAccountService.kt (1)
29-32: LGTM!The new POST endpoint for creating share accounts is properly defined. Returning
HttpResponseallows theDataManagerShareto handle response parsing and error extraction with custom logic.core/network/src/commonMain/kotlin/com/mifos/core/network/model/share/ShareAccountPayload.kt (1)
14-52: The payload model follows proper Kotlin serialization conventions with clear distinction between required and optional fields. However, the non-nullablesavingsAccountIdfield without a default value warrants verification against the actual API contract—if this field is sometimes omitted in API responses, it should beInt?with a nullable default.Cross-reference with:
- API endpoint documentation for share account creation
- Actual request/response examples from integration tests
- Similar payload models in the codebase to ensure consistency
feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/CreateShareAccountViewModel.kt (2)
101-107: LGTM: Loading state handling.The loading indicator is properly shown during the API call by setting
isOverLayLoadingActive = true.
109-120: LGTM: Success state handling with localized message.The success flow correctly clears the loading state, uses a localized string resource for the message, and triggers the UI effect via
launchEffectKey.
| private fun createShareAccount() { | ||
| val shareAccountPayload = ShareAccountPayload( | ||
| clientId = route.clientId, | ||
| productId = state.productOption[state.shareProductIndex!!].id, | ||
| requestedShares = state.totalShares.toInt(), | ||
| externalId = state.externalId, | ||
| submittedDate = state.submissionDate, | ||
| dateFormat = DateHelper.SHORT_MONTH, | ||
| minimumActivePeriod = state.minActivePeriodFreq.toIntOrNull(), | ||
| minimumActivePeriodFrequencyType = if (state.minActivePeriodFreqTypeIdx != null) state.minimumActivePeriodFrequencyTypeOptions[state.minActivePeriodFreqTypeIdx!!].id else null, | ||
| lockinPeriodFrequency = state.lockInPeriodFreq.toIntOrNull(), | ||
| lockinPeriodFrequencyType = if (state.lockInPeriodFreqTypeIdx != null) state.lockInPeriodFrequencyTypeOptions[state.lockInPeriodFreqTypeIdx!!].id else null, | ||
| applicationDate = state.applicationDate, | ||
| allowDividendCalculationForInactiveClients = state.isDividendAllowed, | ||
| charges = state.addedCharges, | ||
| locale = Constants.LOCALE_EN, | ||
| savingsAccountId = state.savingsAccountOptions[state.savingsAccountIdx!!].id, | ||
| ) |
There was a problem hiding this comment.
Potential NPE: Validate nullable indices before payload construction.
The createShareAccount() function uses force-unwrap (!!) on nullable indices. While shareProductIndex and savingsAccountIdx are validated in earlier steps, direct invocation of SubmitShareAccount action could bypass these guards.
Consider adding validation or using safe calls:
private fun createShareAccount() {
+ if (state.shareProductIndex == null || state.savingsAccountIdx == null) {
+ // Handle validation error - show user feedback
+ return
+ }
val shareAccountPayload = ShareAccountPayload(
clientId = route.clientId,
- productId = state.productOption[state.shareProductIndex!!].id,
+ productId = state.productOption[state.shareProductIndex].id,
// ... other fields
- savingsAccountId = state.savingsAccountOptions[state.savingsAccountIdx!!].id,
+ savingsAccountId = state.savingsAccountOptions[state.savingsAccountIdx].id,
)Also, lines 63 and 65 use redundant patterns—after the if (... != null) check, prefer let over !!:
-minimumActivePeriodFrequencyType = if (state.minActivePeriodFreqTypeIdx != null) state.minimumActivePeriodFrequencyTypeOptions[state.minActivePeriodFreqTypeIdx!!].id else null,
+minimumActivePeriodFrequencyType = state.minActivePeriodFreqTypeIdx?.let { state.minimumActivePeriodFrequencyTypeOptions[it].id },| is DataState.Error -> { | ||
| if (dataState.exception is IllegalStateException) { | ||
| mutableStateFlow.update { | ||
| it.copy( | ||
| dialogState = CreateShareAccountState.DialogState.SuccessResponseStatus( | ||
| successStatus = false, | ||
| msg = dataState.message, | ||
| ), | ||
| launchEffectKey = Random.nextInt(), | ||
| isOverLayLoadingActive = false, | ||
| ) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Misleading naming: SuccessResponseStatus used for error states.
Using SuccessResponseStatus(successStatus = false, ...) to represent errors is semantically confusing and could lead to maintenance issues. Consider renaming to a more generic ResponseStatus or creating separate dialog states for success and error:
sealed interface DialogState {
data class AddNewCharge(val edit: Boolean, val index: Int = -1) : DialogState
data object ShowCharges : DialogState
- data class SuccessResponseStatus(val successStatus: Boolean, val msg: String = "") :
- DialogState
+ data class SubmissionResult(val isSuccess: Boolean, val message: String = "") :
+ DialogState
}Also applies to: 621-622
🤖 Prompt for AI Agents
feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/CreateShareAccountViewModel.kt
around lines 77 to 88: the code sets dialogState to
CreateShareAccountState.DialogState.SuccessResponseStatus(successStatus = false,
...) for error branches which is misleading; change the dialog state model to
use a neutral or explicit error type — either rename SuccessResponseStatus to
ResponseStatus (or ResponseResult) and update all usages (including lines
~621-622), or add a distinct DialogState.ErrorResponseStatus with appropriate
fields and set that in error branches; update related consumers (UI renderers,
tests) to handle the new name/type.
| Res.string.feature_share_account_terms_currency to state.currency.orEmpty(), | ||
| Res.string.feature_share_account_terms_current_price to state.currentPrice.orEmpty(), | ||
| Res.string.feature_share_account_terms_total_shares to state.totalShares, | ||
| Res.string.feature_share_account_terms_default_savings_account to state.savingsAccountOptions[state.savingsAccountIdx!!].savingsProductName.orEmpty(), |
There was a problem hiding this comment.
Potential null pointer exception with force unwrap.
Using state.savingsAccountIdx!! will crash if the index is null. If this page can only be reached after validation guarantees a non-null value, consider documenting this precondition. Otherwise, add null-safety.
- Res.string.feature_share_account_terms_default_savings_account to state.savingsAccountOptions[state.savingsAccountIdx!!].savingsProductName.orEmpty(),
+ Res.string.feature_share_account_terms_default_savings_account to (state.savingsAccountIdx?.let { state.savingsAccountOptions.getOrNull(it)?.savingsProductName }.orEmpty()),📝 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.
| Res.string.feature_share_account_terms_default_savings_account to state.savingsAccountOptions[state.savingsAccountIdx!!].savingsProductName.orEmpty(), | |
| Res.string.feature_share_account_terms_default_savings_account to (state.savingsAccountIdx?.let { state.savingsAccountOptions.getOrNull(it)?.savingsProductName }.orEmpty()), |
🤖 Prompt for AI Agents
In
feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/pages/PreviewPage.kt
around line 114, the code force-unwraps state.savingsAccountIdx which can NPE;
replace the force unwrap with a null-safe access (e.g., use
state.savingsAccountIdx?.let {
state.savingsAccountOptions.getOrNull(it)?.savingsProductName.orEmpty() } ?:
""), or explicitly guard earlier with a requireNotNull(state.savingsAccountIdx)
and document the precondition if the index must never be null.
| ) { | ||
| MifosDefaultListingComponentFromStringResources( | ||
| data = mapOf( | ||
| Res.string.feature_share_account_detail_product_name to state.productOption[state.shareProductIndex!!].name, |
There was a problem hiding this comment.
Potential null pointer exception with force unwrap.
Similar to savingsAccountIdx, using state.shareProductIndex!! will crash if null. Add null-safety or ensure the precondition is enforced before reaching this page.
- Res.string.feature_share_account_detail_product_name to state.productOption[state.shareProductIndex!!].name,
+ Res.string.feature_share_account_detail_product_name to (state.shareProductIndex?.let { state.productOption.getOrNull(it)?.name }.orEmpty()),📝 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.
| Res.string.feature_share_account_detail_product_name to state.productOption[state.shareProductIndex!!].name, | |
| Res.string.feature_share_account_detail_product_name to (state.shareProductIndex?.let { state.productOption.getOrNull(it)?.name }.orEmpty()), |
🤖 Prompt for AI Agents
In
feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/pages/PreviewPage.kt
around line 130, the code force-unwraps state.shareProductIndex (!!) which can
throw an NPE; replace the force-unwrap with a null-safe approach or enforce the
precondition: either (a) use a safe-access and handle null (e.g.,
state.shareProductIndex?.let { idx -> use idx } ?: return or provide a fallback)
or (b) assert the invariant at an earlier point with
requireNotNull(state.shareProductIndex) { "shareProductIndex must be set before
PreviewPage" } so the value is guaranteed non-null when accessed. Ensure the
chosen fix avoids runtime NPE and preserves correct UI flow.
|
|
||
| throw IllegalStateException(errorMsg) | ||
| } else { | ||
| val json = Json { ignoreUnknownKeys = true } |
There was a problem hiding this comment.
isn't this already set in the Ktorfit client when it was created?
I have looked for a while but it must be.
3 participants
Fixes - Jira-#546
IMG_5328.MOV
Summary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.