feat(shareAccount): implement terms page for share account. #2531
Conversation
WalkthroughAdds an optional productId to share-template retrieval across service → datamanager → repository, extends network share models with currency/options, makes Terms UI state-driven with new fields/date pickers, updates ViewModel/state/actions for the form, and renames/introduces feature-specific string resources. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Screen as CreateShareAccountScreen
participant VM as CreateShareAccountViewModel
participant Repo as ShareAccountRepository
participant DataMgr as DataManagerShare
participant API as ShareAccountService
participant Terms as TermsPage
User->>Screen: Select product
Screen->>VM: OnShareProductChange(productId)
VM->>VM: loadShareTemplateFromProduct(clientId, productId)
VM->>Repo: getShareTemplate(clientId, productId)
Repo->>DataMgr: getShareTemplate(clientId, productId)
DataMgr->>API: shareProductTemplate(clientId, productId)
API-->>DataMgr: Flow<ShareTemplate>
DataMgr-->>Repo: Flow<ShareTemplate>
Repo-->>VM: Flow<DataState<ShareTemplate>>
VM-->>Screen: Update state (currency, price, options...)
Screen->>Terms: Render(state, onAction)
User->>Terms: Interact (inputs, pickers)
Terms->>VM: onAction(...) (e.g., OnApplicationDateChange, OnTotalSharesChange)
Terms->>VM: onAction(OnTermsNext)
VM->>VM: Validate & advance step
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 |
|
@sam-arth07 I you don't work on it please tell me I will complete it |
@Arinyadav1 I am working on it, i just started yesterday night. I guess there are some tickets that have no progress in recurring accounts flow. : You can select one of these |
sam-arth07
changed the title
…ker fixes Co-authored-by: sam-arth07 <82860337+sam-arth07@users.noreply.github.com>
Co-authored-by: sam-arth07 <82860337+sam-arth07@users.noreply.github.com>
…toString calls Co-authored-by: sam-arth07 <82860337+sam-arth07@users.noreply.github.com>
Co-authored-by: sam-arth07 <82860337+sam-arth07@users.noreply.github.com>
Co-authored-by: sam-arth07 <82860337+sam-arth07@users.noreply.github.com>
… product and enhance state management
…dling in terms page
![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
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)
156-169: Critical: Race condition between async data loading and navigation.The
handleOnDetailNextfunction callsloadShareTemplateFromProduct(which launches an asynchronous coroutine) and then immediately callsmoveToNextStep()without waiting for the data to load. This creates a race condition where the Terms page is rendered before the required data (currency, currentPrice, savingsAccountOptions, frequency options) is available.Impact:
- Users will see loading indicators or empty/broken UI on the Terms page
- Potential null pointer exceptions if Terms page assumes data is ready
- Poor user experience
Solution: Only advance to the next step after the data successfully loads:
private fun handleOnDetailNext() { if (state.shareProductIndex == null) { mutableStateFlow.update { it.copy( shareProductError = TextFieldsValidator.stringValidator(""), ) } } else { state.selectedProduct?.id?.let { productId -> loadShareTemplateFromProduct(client = route.clientId, productId = productId) } - moveToNextStep() } }Then modify
loadShareTemplateFromProductto callmoveToNextStep()only in the Success case:is DataState.Success -> { mutableStateFlow.update { it.copy( screenState = ShareAccountState.ScreenState.Success, currency = dataState.data.currency?.name, currentPrice = dataState.data.currentMarketPrice?.toString(), savingsAccountOptions = dataState.data.savingsAccountOptions.orEmpty(), lockInPeriodFrequencyTypeOptions = dataState.data.lockinPeriodFrequencyTypeOptions.orEmpty(), minimumActivePeriodFrequencyTypeOptions = dataState.data.minimumActivePeriodFrequencyTypeOptions.orEmpty(), ) } + moveToNextStep() }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
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/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/component/MifosOutlinedTextField.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/ProductOption.kt(1 hunks)core/network/src/commonMain/kotlin/com/mifos/core/network/model/share/ShareTemplate.kt(2 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(1 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/CreateShareAccountViewModel.kt(10 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/pages/DetailsPage.kt(3 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/pages/TermsPage.kt(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/pages/TermsPage.kt (4)
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 (5)
MifosOutlinedTextField(66-124)MifosOutlinedTextField(126-243)MifosOutlinedTextField(245-308)MifosOutlinedTextField(415-480)MifosDatePickerTextField(363-413)core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosCheckBox.kt (1)
MifosCheckBox(24-46)core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosTwoButtonRow.kt (1)
MifosTwoButtonRow(31-91)
🔇 Additional comments (5)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/pages/DetailsPage.kt (1)
61-130: LGTM! Clean refactoring for multi-date flow.The renaming of date picker actions from generic (
OnDateChange,OnOpenDatePicker) to submission-specific (OnSubmissionDateChange,OnOpenSubmissionDatePicker) improves clarity and aligns well with the addition of the application date picker in the terms page. The date validation change from "within last 24 hours" to "up to now" is also more intuitive.core/network/src/commonMain/kotlin/com/mifos/core/network/model/share/ShareTemplate.kt (1)
12-68: LGTM! Well-structured model extension.The additions to
ShareTemplateand the newSavingsAccountOptionandFrequencyTypeOptiondata classes are properly structured:
- All new fields are nullable with safe defaults, ensuring backward compatibility
- Serialization annotations correctly map to API field names
- New data classes follow the same pattern as existing code
feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/CreateShareAccountViewModel.kt (2)
171-215: LGTM! Well-structured validation logic.The
handleOnTermsNextvalidation properly:
- Validates required fields (totalShares, savingsAccountIdx)
- Conditionally validates optional fields (frequency values only if provided)
- Collects all errors before updating state (better UX than showing one error at a time)
- Only advances to the next step if validation passes
366-436: LGTM! Comprehensive state and action definitions.The expanded
ShareAccountStateandShareAccountActionproperly support the new Terms page functionality:
- State fields have sensible defaults
- The
selectedProductcomputed property safely handles null indices- Actions cover all necessary UI interactions
- Follows existing patterns in the codebase
feature/client/src/commonMain/composeResources/values/strings.xml (1)
549-559: No duplicates found—review comment is incorrect.The verification script found 10 unique string resource keys (lines 549–558), each defined exactly once. The original review comment's claim that these strings "appear to be defined twice" is not supported by the file contents. No duplicate string resource keys exist in this section that require removal or remediation.
Likely an incorrect or invalid review comment.
.../client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/pages/TermsPage.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/pages/TermsPage.kt (1)
162-167: Null suffix handling has been fixed.The previous review concern about nullable
savingsProductNameconcatenating as"null"has been properly addressed with.orEmpty()on line 163. Account numbers now display without a suffix when the product name is absent.
🧹 Nitpick comments (1)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/pages/TermsPage.kt (1)
118-124: Replace dropdown with text field for display-only currency.Using a dropdown component with an empty options list and
enabled = falseis unconventional for read-only display. A standardMifosOutlinedTextField(like current price on lines 126–133) would be clearer and more consistent.-MifosTextFieldDropdown( - value = state.currency.orEmpty(), - onValueChanged = {}, - onOptionSelected = { _, _ -> }, - options = emptyList(), - label = stringResource(Res.string.share_account_terms_currency), - enabled = false, -) +MifosOutlinedTextField( + value = state.currency.orEmpty(), + onValueChange = {}, + label = stringResource(Res.string.share_account_terms_currency), + config = MifosTextFieldConfig( + enabled = false, + ), +)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/pages/TermsPage.kt(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/pages/TermsPage.kt (4)
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 (5)
MifosOutlinedTextField(66-124)MifosOutlinedTextField(126-243)MifosOutlinedTextField(245-308)MifosOutlinedTextField(415-480)MifosDatePickerTextField(363-413)core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosCheckBox.kt (1)
MifosCheckBox(24-46)core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosTwoButtonRow.kt (1)
MifosTwoButtonRow(31-91)
🔇 Additional comments (4)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/pages/TermsPage.kt (4)
12-60: LGTM! Imports are clean and properly organized.All necessary dependencies are imported, and experimental API opt-ins are correctly declared for DatePicker and Clock.System usage.
77-106: LGTM! DatePickerDialog is correctly implemented.The dialog properly handles the show/hide state, date selection with null safety, and dispatches appropriate actions. Button labels are correctly localized.
209-225: Good UX: Type dropdowns conditionally enabled based on frequency.The pattern of enabling the frequency-type dropdown only when the corresponding frequency field has a value (lines 222 and 262) provides clear guidance to users about the dependency between these fields.
Also applies to: 249-265
267-277: LGTM! Navigation buttons are properly wired.The button row is correctly positioned outside the scrollable content and dispatches appropriate navigation actions. Form validation likely occurs in the action handler, which is an acceptable pattern.
| val applicationDatePickerState = rememberDatePickerState( | ||
| initialSelectedDateMillis = Clock.System.now().toEpochMilliseconds(), | ||
| selectableDates = object : SelectableDates { | ||
| override fun isSelectableDate(utcTimeMillis: Long): Boolean { | ||
| return utcTimeMillis <= Clock.System.now().toEpochMilliseconds() | ||
| } | ||
| }, | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
DatePicker should initialize from state.applicationDate when present.
The picker always starts at the current time, even if state.applicationDate already holds a previously selected date. When users navigate back to this page, their prior selection is lost from the picker UI (though still present in state), creating a confusing experience.
Parse state.applicationDate when initializing the picker:
val applicationDatePickerState = rememberDatePickerState(
- initialSelectedDateMillis = Clock.System.now().toEpochMilliseconds(),
+ initialSelectedDateMillis = if (state.applicationDate.isNotBlank()) {
+ DateHelper.getLongFromString(state.applicationDate)
+ } else {
+ Clock.System.now().toEpochMilliseconds()
+ },
selectableDates = object : SelectableDates {
override fun isSelectableDate(utcTimeMillis: Long): Boolean {
return utcTimeMillis <= Clock.System.now().toEpochMilliseconds()
}
},
)Note: Verify that DateHelper.getLongFromString exists and handles the date format used by getDateAsStringFromLong. If not available, you'll need to add this conversion utility.
Run this script to check for the inverse conversion utility:
🏁 Script executed:
#!/bin/bash
# Description: Verify DateHelper has a method to parse date strings back to Long
# Search for getLongFromString or similar string-to-long date conversion methods
ast-grep --pattern 'fun getLongFromString($$$) { $$$ }'
# Also search with ripgrep for any string-to-long date conversion in DateHelper
rg -A 5 'fun.*String.*Long|fun.*parse.*date' --type=kt -g '**/DateHelper.kt'Length of output: 26
🏁 Script executed:
#!/bin/bash
# Description: Verify DateHelper methods and date conversion utilities
# Find DateHelper file
echo "=== Locating DateHelper.kt ==="
fd 'DateHelper.kt' --type f
# Search for date conversion methods without type restriction
echo ""
echo "=== Searching for date conversion methods in Kotlin files ==="
rg 'fun.*String.*Long|fun.*Long.*String|fun.*getDate|fun.*parse.*[Dd]ate' --type kotlin -A 3
# Search more broadly for DateHelper class
echo ""
echo "=== Searching for DateHelper class definition ==="
rg 'object DateHelper|class DateHelper' --type kotlin -A 20
# Look for state.applicationDate usage
echo ""
echo "=== Checking state.applicationDate usage in TermsPage ==="
rg 'applicationDate' feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/pages/TermsPage.kt -A 2 -B 2Length of output: 9117
🏁 Script executed:
#!/bin/bash
# Description: View full DateHelper.kt to see all available conversion methods
cat core/common/src/commonMain/kotlin/com/mifos/core/common/utils/DateHelper.ktLength of output: 16490
🏁 Script executed:
#!/bin/bash
# Description: Check full TermsPage context for date handling
cat feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/pages/TermsPage.ktLength of output: 12941
DatePicker initializes with current date instead of previously selected date; suggested fix references non-existent method.
The UX concern is valid: when users navigate back, the picker resets to today even though state.applicationDate contains their prior selection. However, the suggested fix references DateHelper.getLongFromString(), which does not exist in the codebase and will cause a compilation error.
To restore the previously selected date, parse state.applicationDate (format: "dd-MM-yyyy") back to milliseconds:
val applicationDatePickerState = rememberDatePickerState(
initialSelectedDateMillis = if (state.applicationDate.isNotBlank()) {
try {
val (day, month, year) = state.applicationDate.split("-").map { it.toInt() }
LocalDate(year, month, day)
.atStartOfDayIn(TimeZone.UTC)
.toEpochMilliseconds()
} catch (e: Exception) {
Clock.System.now().toEpochMilliseconds()
}
} else {
Clock.System.now().toEpochMilliseconds()
},
// ... rest of configuration
)Alternatively, add a helper method to DateHelper for bidirectional conversion (e.g., getLongFromDateString(dateString: String): Long).
| @SerialName(value = "clientName") | ||
| val clientName: String, | ||
|
|
||
| @SerialName("currency") |
There was a problem hiding this comment.
@SerialName is required only if custom name is different from the property name. Please remove them wherever it is redundant.
Reference: kotlinx.serialization official docs
| @SerialName("shortName") | ||
| val shortName: String, | ||
|
|
||
| @SerialName("totalShares") |
| <string name="share_account_detail_date_select">Select</string> | ||
| <string name="share_account_detail_date_cancel">Cancel</string> | ||
|
|
||
| <string name="share_account_terms_currency">Currency</string> |
There was a problem hiding this comment.
It is better to follow a single naming convention:
| <string name="share_account_terms_currency">Currency</string> | |
| <string name="feature_share_account_terms_currency">Currency</string> |
| @GET("accounts/" + APIEndPoint.SHARE + "/template") | ||
| fun shareProductTemplate( | ||
| @Query("clientId") clientId: Int, | ||
| ): Flow<ShareTemplate> | ||
|
|
||
| @GET("accounts/" + APIEndPoint.SHARE + "/template") | ||
| fun shareProductTemplateFromProduct( | ||
| @Query("clientId") clientId: Int, | ||
| @Query("productId") productId: Int, | ||
| ): Flow<ShareTemplate> |
There was a problem hiding this comment.
Both functions have the same API endpoints, consider a single method with optional productId field.
| override fun getShareTemplate(clientId: Int): Flow<DataState<ShareTemplate>> { | ||
| return dataManagerShare.getShareTemplate(clientId).asDataStateFlow() | ||
| } | ||
|
|
||
| override fun getShareTemplateFromProduct( | ||
| clientId: Int, | ||
| productId: Int, | ||
| ): Flow<DataState<ShareTemplate>> { | ||
| return dataManagerShare.getShareTemplateFromProduct(clientId, productId).asDataStateFlow() | ||
| } |
…efix and update share template retrieval method
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/pages/DetailsPage.kt (1)
57-64: Date picker should restore previous submission date from state.Similar to TermsPage, this date picker always initializes to today (line 58) instead of restoring
state.submissionDatewhen users navigate back. This creates inconsistent UX where the text field displays the previously selected date but the picker opens to a different date.Consider initializing from state:
val submissionDatePickerState = rememberDatePickerState( - initialSelectedDateMillis = Clock.System.now().toEpochMilliseconds(), + initialSelectedDateMillis = if (state.submissionDate.isNotBlank()) { + try { + DateHelper.getDateAsLongFromString(state.submissionDate) + } catch (e: Exception) { + Clock.System.now().toEpochMilliseconds() + } + } else { + Clock.System.now().toEpochMilliseconds() + }, selectableDates = object : SelectableDates {feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/CreateShareAccountViewModel.kt (1)
156-169: Step advances before template loading completes.Lines 164-168 call the asynchronous
loadShareTemplateFromProductand immediately advance to the next step without waiting for the load to complete. This causes the Terms page to display prematurely, potentially showing a loading spinner or error state before the user has seen it transition.Consider advancing the step only after the template loads successfully:
private fun handleOnDetailNext() { if (state.shareProductIndex == null) { mutableStateFlow.update { it.copy( shareProductError = TextFieldsValidator.stringValidator(""), ) } } else { - state.selectedProduct?.id?.let { productId -> - loadShareTemplateFromProduct(client = route.clientId, productId = productId) - } - moveToNextStep() + val productId = state.selectedProduct?.id + if (productId != null) { + viewModelScope.launch { + val online = networkMonitor.isOnline.first() + if (online) { + mutableStateFlow.update { it.copy(screenState = ShareAccountState.ScreenState.Loading) } + repository.getShareTemplate(route.clientId, productId).collect { dataState -> + when (dataState) { + is DataState.Success -> { + mutableStateFlow.update { + it.copy( + screenState = ShareAccountState.ScreenState.Success, + currency = dataState.data.currency?.name, + currentPrice = dataState.data.currentMarketPrice?.toString(), + savingsAccountOptions = dataState.data.savingsAccountOptions.orEmpty(), + lockInPeriodFrequencyTypeOptions = dataState.data.lockinPeriodFrequencyTypeOptions.orEmpty(), + minimumActivePeriodFrequencyTypeOptions = dataState.data.minimumActivePeriodFrequencyTypeOptions.orEmpty(), + ) + } + moveToNextStep() + } + is DataState.Error -> { + mutableStateFlow.update { + it.copy(screenState = ShareAccountState.ScreenState.Error(dataState.message)) + } + } + DataState.Loading -> { + mutableStateFlow.update { + it.copy(screenState = ShareAccountState.ScreenState.Loading) + } + } + } + } + } else { + mutableStateFlow.update { + it.copy(screenState = ShareAccountState.ScreenState.Error(getString(Res.string.feature_client_error_network_not_available))) + } + } + } + } } }Or, keep the current structure and only call
moveToNextStep()from within theDataState.Successbranch ofloadShareTemplateFromProduct.
♻️ Duplicate comments (1)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/pages/TermsPage.kt (1)
68-75: Date picker doesn't restore previously selected date.When users navigate back to this page, the picker resets to today even though
state.applicationDatecontains their prior selection. This creates a confusing UX where the text field shows one date but the picker opens to a different date.To restore the previous selection, parse
state.applicationDatewhen initializing the picker:val applicationDatePickerState = rememberDatePickerState( - initialSelectedDateMillis = Clock.System.now().toEpochMilliseconds(), + initialSelectedDateMillis = if (state.applicationDate.isNotBlank()) { + try { + DateHelper.getDateAsLongFromString(state.applicationDate) + } catch (e: Exception) { + Clock.System.now().toEpochMilliseconds() + } + } else { + Clock.System.now().toEpochMilliseconds() + }, selectableDates = object : SelectableDates {Note: Verify that
DateHelper.getDateAsLongFromString(or similar) exists to parse the "dd-MM-yyyy" format back to milliseconds, or add such a utility method.#!/bin/bash # Verify DateHelper has string-to-long date conversion rg -n "fun.*String.*Long|fun.*parse" --type=kt -g '**/DateHelper.kt' -A 3
🧹 Nitpick comments (2)
feature/client/src/commonMain/composeResources/values/strings.xml (1)
542-568: Maintain title case consistency for field labels.Lines 555–564 define the Terms page field labels. Most follow title case (e.g., "Currency", "Current Price", "Total Number of Shares"), but line 560 uses mixed case:
<string name="feature_share_account_terms_allow_dividends">Allow dividends for inactive clients</string>Align it with the rest for consistency:
<string name="feature_share_account_terms_allow_dividends">Allow Dividends for Inactive Clients</string>Otherwise, the naming convention
feature_share_account_*is consistent with the previous review feedback and aligns well with the existingfeature_client_*pattern in the file.feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/CreateShareAccountViewModel.kt (1)
171-215: Consider validating applicationDate in terms validation.The validation logic checks totalShares, savingsAccountIdx, and frequency fields, but
applicationDateis not validated despite having anapplicationDateErrorfield in the state (line 392). If the date picker always provides valid dates and empty dates are acceptable, this is fine; otherwise, consider adding validation.If applicationDate is required, add validation:
if (state.savingsAccountIdx == null) { newState = newState.copy(savingsAccountError = TextFieldsValidator.stringValidator("")) hasError = true } + + if (state.applicationDate.isBlank()) { + newState = newState.copy(applicationDateError = TextFieldsValidator.stringValidator("")) + hasError = true + } if (state.minActivePeriodFreq.isNotBlank()) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
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/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerShare.kt(1 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/createShareAccount/CreateShareAccountScreen.kt(2 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/CreateShareAccountViewModel.kt(10 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/pages/DetailsPage.kt(4 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/pages/TermsPage.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- core/network/src/commonMain/kotlin/com/mifos/core/network/services/ShareAccountService.kt
- core/data/src/commonMain/kotlin/com/mifos/core/data/repositoryImp/ShareAccountRepositoryImpl.kt
- core/network/src/commonMain/kotlin/com/mifos/core/network/datamanager/DataManagerShare.kt
🧰 Additional context used
🧬 Code graph analysis (3)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/pages/TermsPage.kt (4)
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 (5)
MifosOutlinedTextField(66-124)MifosOutlinedTextField(126-243)MifosOutlinedTextField(245-308)MifosOutlinedTextField(415-480)MifosDatePickerTextField(363-413)core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosCheckBox.kt (1)
MifosCheckBox(24-46)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/pages/DetailsPage.kt (2)
core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/component/MifosOutlinedTextField.kt (1)
MifosDatePickerTextField(363-413)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 (2)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/pages/DetailsPage.kt (1)
DetailsPage(50-152)feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/pages/TermsPage.kt (1)
TermsPage(61-279)
🔇 Additional comments (9)
core/network/src/commonMain/kotlin/com/mifos/core/network/model/share/ShareTemplate.kt (1)
12-54: LGTM! Model extensions support new Terms page requirements.The additions to ShareTemplate and the new option data classes (SavingsAccountOption, FrequencyTypeOption) properly support the expanded Terms UI. All new fields use sensible defaults for backward compatibility, and the
@SerialNameannotation on line 28 is correctly used since the JSON keyclientSavingsAccountsdiffers from the property namesavingsAccountOptions.core/data/src/commonMain/kotlin/com/mifos/core/data/repository/ShareAccountRepository.kt (1)
18-18: LGTM! Interface updated to support product-specific templates.The addition of the optional
productIdparameter enables product-specific template retrieval while maintaining backward compatibility through its nullable type.core/network/src/commonMain/kotlin/com/mifos/core/network/model/share/ProductOption.kt (1)
24-42: LGTM! Pricing fields added to ProductOption.The addition of
currencyandunitPricefields, along with the newProductCurrencydata class, properly extends the product data model to support pricing display in the UI. The redundant@SerialNameannotations mentioned in past reviews have been correctly removed.feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/CreateShareAccountScreen.kt (1)
13-16: LGTM! Screen updated for state-driven TermsPage.The migration of TermsPage to a state-driven pattern (lines 81-84) improves consistency with modern Compose practices. The string resource updates to
feature_share_account_*variants maintain proper namespacing.Also applies to: 74-74, 80-84, 86-86, 91-91
feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/pages/TermsPage.kt (2)
162-164: Good fix for null suffix issue.The use of
.orEmpty()on line 163 properly resolves the past review concern about displaying "null" whensavingsProductNameis absent.
63-279: Well-structured state-driven form implementation.The conversion to a fully state-driven composable with comprehensive field coverage, validation error display, and action dispatching follows best practices for Compose. The form properly handles currency display, savings account selection, date pickers, checkbox for dividends, and frequency configurations.
feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/pages/DetailsPage.kt (1)
13-20: LGTM! Consistent updates to string resources and action naming.The string resource migrations to
feature_share_account_*variants and action renamings (OnOpenSubmissionDatePicker,OnSubmissionDateChange) maintain consistency across the codebase.Also applies to: 61-61, 66-66, 69-69, 74-74, 77-77, 83-83, 88-88, 90-90, 102-102, 120-120, 125-127, 136-136, 141-142
feature/client/src/commonMain/kotlin/com/mifos/feature/client/createShareAccount/CreateShareAccountViewModel.kt (2)
48-92: LGTM! Template loading method properly handles offline state.The
loadShareTemplateFromProductmethod correctly checks online status, emits appropriate loading/error/success states, and populates all required fields (currency, price, options) from the template data.
21-21: LGTM! Action handlers and state expanded to support multi-step Terms flow.The expanded state includes all necessary fields (savings options, frequency options, currency, prices, validation errors) and the action handlers properly update state while clearing related error fields. The new
selectedProductcomputed property (line 405) provides clean access to the selected product option.Also applies to: 23-23, 217-436
biplab1
left a comment
biplab1
left a comment
There was a problem hiding this comment.
Looks good to me. This can be merged.
4 participants
Fixes - Jira-#544
ShareTerms.webm
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
New Features
Bug Fixes