fix: add missing gender, clientType and clientClassification in ClientProfileDetails screen#2578
Conversation
📝 WalkthroughWalkthroughAdds client gender, type, and classification across layers: new Room entities and converters, new network DTOs and response fields, mapper mappings between network and DB sub-entities, DB entity extensions, and UI/viewmodel initialization and display adjustments. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@core/network/src/commonMain/kotlin/com/mifos/core/network/mappers/clients/ClientMapper.kt`:
- Around line 98-109: In mapToEntity, avoid always constructing empty option
objects; change the assignments for gender, clientType, and clientClassification
to conditional creations using Kotlin's safe-call with let (e.g.,
domainModel.gender?.let { GetGenderOptions(id = it.id?.toLong(), name = it.name)
}) so each field becomes null when the source is null; apply the same pattern
for clientType (GetClientTypeOptions) and clientClassification
(GetClientClassificationOptions) in the ClientMapper.mapToEntity function.
🧹 Nitpick comments (1)
core/database/src/commonMain/kotlin/com/mifos/room/typeconverters/CustomTypeConverters.kt (1)
187-215: LGTM!The new TypeConverter pairs follow the established pattern in this file. They correctly handle nullable values and use
kotlinx.serializationfor JSON encoding/decoding.Minor observation: These converters use explicit type parameters (e.g.,
Json.decodeFromString<ClientGenderEntity>(it)) while some existing converters rely on type inference (e.g.,toClientStatusat line 184). Both approaches work correctly, but you may consider aligning the style for consistency.
core/network/src/commonMain/kotlin/com/mifos/core/network/mappers/clients/ClientMapper.kt
Outdated
Show resolved
Hide resolved
c017e90 to
da2dbe1
Compare
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 (1)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientEditDetails/ClientEditDetailsScreen.kt (1)
621-648: Enable selection even when values are missing.With the new defaults (empty strings),
enabled = clientType.isNotEmpty()/clientClassification.isNotEmpty()prevents users from filling missing data. Gate on list availability instead.✅ Proposed fix
- MifosTextFieldDropdown( - enabled = clientType.isNotEmpty(), + MifosTextFieldDropdown( + enabled = list.isNotEmpty(), value = clientType, onValueChanged = { clientType = it }, onOptionSelected = { index, value -> clientType = value selectedClientTypeId = list[index].id }, label = stringResource(Res.string.feature_client_client), options = list.sortedBy { it.id }.map { it.name }, readOnly = true, )- MifosTextFieldDropdown( - enabled = clientClassification.isNotEmpty(), + MifosTextFieldDropdown( + enabled = list.isNotEmpty(), value = clientClassification, onValueChanged = { clientClassification = it }, onOptionSelected = { index, value -> clientClassification = value selectedClientClassificationId = list[index].id }, label = stringResource(Res.string.feature_client_client_classification), options = list.sortedBy { it.id }.map { it.name }, readOnly = true, )
🧹 Nitpick comments (1)
core/network/src/commonMain/kotlin/com/mifos/core/network/mappers/clients/ClientMapper.kt (1)
104-115: Inconsistent use ofitinside.letblocks forclientTypeandclientClassification.The
gendermapping correctly usesit.idandit.name(lines 100-101), butclientTypeandclientClassificationredundantly access the outer scope (domainModel.clientType?.id,domainModel.clientClassification?.id) instead of using theitreference. While functionally equivalent, this is inconsistent and defeats the clarity benefit of using.let.♻️ Proposed fix for consistency
clientType = domainModel.clientType?.let { GetClientTypeOptions( - id = domainModel.clientType?.id?.toLong(), - name = domainModel.clientType?.name, + id = it.id?.toLong(), + name = it.name, ) }, clientClassification = domainModel.clientClassification?.let { GetClientClassificationOptions( - id = domainModel.clientClassification?.id?.toLong(), - name = domainModel.clientClassification?.name, + id = it.id?.toLong(), + name = it.name, ) },
…tProfileDetails screen
da2dbe1 to
d93ee45
Compare
|
@amanna13 In the edit screen, when the gender and classification types are not available in the back-end, are dropdowns disabled automatically? |
.../database/src/commonMain/kotlin/com/mifos/room/entities/client/ClientClassificationEntity.kt
Show resolved
Hide resolved
@biplab1 For gender the dropdown is available even if its not available. but for WhatsApp.Video.2026-01-19.at.7.09.40.PM.mp4You can have a look into this. Though these were there previously, I didn't touch these section in the code. |
|
I had checked it earlier, after I added gender and client classification codes in the back-end via web-app, the Gender drop-down menu was working and Client Classification was still not working, the last one is what you have fixed. So, currently make sure that both the cases are handled properly:
|
|
Can you check the order of the codes sent in the API response? I believe the back-end sorts the codes based on Also, make sure the API responses are completely consumed by the data classes even if those fields are not used in the app currently. |
|
@biplab1 yes, you are right. I checked it, the back-end automatically sorts the codes based on |
|
@amanna13 Yes, please go ahead, the sorting code in the front-end is redundant. |
@biplab1 I have tested with postman, The API response returns this below (for Notice that these don't have those extra fields configured yet - |
|
Understood,. Can you push the latest updates so that I can do a final review? |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientEditDetails/ClientEditDetailsScreen.kt`:
- Around line 491-503: The dropdowns in ClientEditDetailsScreen are currently
enabled based on the current selected value (e.g., gender.isNotEmpty()) which
prevents selection when a value is missing; update each MifosTextFieldDropdown
(e.g., the gender block inside clientTemplate.genderOptions?.let { list -> ...
}) to use the options list emptiness as the enabled condition (enabled =
list.isNotEmpty()) instead of checking the field value, and apply the same
change to the other similar blocks referenced (the blocks around the 621-633 and
636-648 ranges) so all dropdowns enable when options are available.
.../src/commonMain/kotlin/com/mifos/feature/client/clientEditDetails/ClientEditDetailsScreen.kt
Show resolved
Hide resolved
|
@biplab1 Updated screenshot attached below (This will be in the case when the options are not configured in the backend) WhatsApp.Video.2026-01-21.at.8.22.14.PM.mp4When the options are configured Screen_recording_20260121_215431.mp4 |
biplab1
left a comment
There was a problem hiding this comment.
Some minor changes requested.
Looks good to me. This can be merged.
Fixes - Jira-#579
ClientGenderEntity,ClientTypeEntity,ClientClassificationEntityin core/databaseCustomTypeConvertersfor serializing/deserializing these entities to/from JSONGetGenderOptions,GetClientTypeOptions,GetClientClassificationOptionsincore/networkand updated theClientMapperaccordingly.Note
GenderOptions,ClientTypeOptions,ClientClassificationOptions.ktthough present in thecore/model/objects/organisationswas never used. For consistency we have added new models in thecore/networkand kept the old files as is.Also did respective changes in the ClientEditDetailsScreen
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
✏️ Tip: You can customize this high-level summary in your review settings.