chore: remove fieldUiModel from SearchParameters state [skip size]#4656
chore: remove fieldUiModel from SearchParameters state [skip size]#4656xavimolloy wants to merge 6 commits intodevelopfrom
Conversation
xavimolloy
changed the title
There was a problem hiding this comment.
Pull request overview
This pull request refactors the search parameters functionality by replacing FieldUiModel with TrackerInputModel in the search parameters state. The change aims to decouple the search functionality from the form-specific model and use a more appropriate tracker-specific model instead.
Changes:
- Replaced
FieldUiModelwithTrackerInputModelinSearchParametersUiStateand related code - Added new mapping logic (
mapToTrackerModel) inSearchRepositoryImplKtto convert tracked entity attributes toTrackerInputModel - Updated
SearchParametersScreento work directly withTrackerInputModeland removed the callback pattern - Added
UiEventTypesProviderinjection and utility functions for type conversions
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| tracker/src/commonMain/kotlin/org/dhis2/tracker/ui/input/model/TrackerInputModel.kt | Added orgUnitSelectorScope field to support organization unit selection |
| app/src/main/java/org/dhis2/usescases/searchTrackEntity/searchparameters/model/SearchParametersUiState.kt | Changed items type from FieldUiModel to TrackerInputModel |
| app/src/main/java/org/dhis2/usescases/searchTrackEntity/searchparameters/mapper/ParameterInputModelMapper.kt | Renamed function and added TODOs for incomplete functionality |
| app/src/main/java/org/dhis2/usescases/searchTrackEntity/searchparameters/SearchParametersScreen.kt | Removed FieldUiModel dependency, commented out QR scanning and next field navigation |
| app/src/main/java/org/dhis2/usescases/searchTrackEntity/SearchTEModule.java | Added UiEventTypesProvider dependency injection |
| app/src/main/java/org/dhis2/usescases/searchTrackEntity/SearchTEIViewModel.kt | Updated to work with TrackerInputModel, changed type handling logic |
| app/src/main/java/org/dhis2/usescases/searchTrackEntity/SearchRepositoryKt.kt | Updated interface to return TrackerInputModel and added validation/conversion methods |
| app/src/main/java/org/dhis2/usescases/searchTrackEntity/SearchRepositoryImplKt.kt | Added mapToTrackerModel mapper and trackerValueTypeToSDKValueType conversion, incomplete validateValue implementation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| { | ||
| when (inputType) { | ||
| TrackerInputType.DATE -> { | ||
| ValueType.DATE.validator.validate(value) | ||
| } | ||
|
|
||
| else -> { | ||
| false | ||
| } |
There was a problem hiding this comment.
The validateValue function returns a lambda instead of executing it. The function body is wrapped in curly braces that create a lambda, but the lambda is never invoked. This means the validation logic never runs, and the function always returns a lambda object instead of the validation result. Remove the outer curly braces on line 344 and the closing brace on line 354 so that the when expression is directly returned.
| { | |
| when (inputType) { | |
| TrackerInputType.DATE -> { | |
| ValueType.DATE.validator.validate(value) | |
| } | |
| else -> { | |
| false | |
| } | |
| when (inputType) { | |
| TrackerInputType.DATE -> { | |
| ValueType.DATE.validator.validate(value) | |
| } | |
| else -> { | |
| false |
app/src/main/java/org/dhis2/usescases/searchTrackEntity/SearchRepositoryImplKt.kt
Show resolved
Hide resolved
app/src/main/java/org/dhis2/usescases/searchTrackEntity/SearchTEIViewModel.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/org/dhis2/usescases/searchTrackEntity/SearchRepositoryImplKt.kt
Outdated
Show resolved
Hide resolved
...c/main/java/org/dhis2/usescases/searchTrackEntity/searchparameters/SearchParametersScreen.kt
Outdated
Show resolved
Hide resolved
...c/main/java/org/dhis2/usescases/searchTrackEntity/searchparameters/SearchParametersScreen.kt
Outdated
Show resolved
Hide resolved
...c/main/java/org/dhis2/usescases/searchTrackEntity/searchparameters/SearchParametersScreen.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/org/dhis2/usescases/searchTrackEntity/SearchRepositoryImplKt.kt
Outdated
Show resolved
Hide resolved
...a/org/dhis2/usescases/searchTrackEntity/searchparameters/mapper/ParameterInputModelMapper.kt
Outdated
Show resolved
Hide resolved
* feat: Create SearchParametersRepository.kt * feat: Create FetchSearchParameters.kt * feat: Add horizontal and vertical checkboxes * refctor: Remove get search params from SearchRepositoryImplKt.kt * feat: Inject FetchSearchParameters in SearchTEIViewModel.kt
…skip size] (#4662) * feat: [ANDROAPP-7470] Add SearchScreenUiEvent and other tracker events * feat: [ANDROAPP-7470] Move Search Parameters screen to tracker module, refactor ols screen file to provider * feat: [ANDROAPP-7470] Implement new actions in viewmodel and other changes by extension * feat: [ANDROAPP-7470] Adapt Searchtei viewmodel test
* Add some future annotations * feat: create FetchOptionSetOptions.kt * doc: comment over TrackerInputType.kt * implementing optionset * Move composable operations to TrackerInputModel.kt * feat: integrate optionset in new screen
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 47 out of 47 changed files in this pull request and generated 11 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fun onItemClick(fieldUid: FieldUid) { | ||
| searchParametersUiState | ||
| .copy( | ||
| items = | ||
| searchParametersUiState.items.map { | ||
| if (it.uid == fieldUid) { | ||
| it.copy(focused = true) | ||
| } else { | ||
| it | ||
| } | ||
| }, | ||
| ).let { | ||
| searchParametersUiState = it | ||
| } |
There was a problem hiding this comment.
onItemClick sets focused = true on the tapped field but never clears focus on other items, so multiple items can remain focused at the same time. Update this mapping to set focused = (it.uid == fieldUid) (or explicitly clear others) to keep focus mutually exclusive.
| { | ||
| when (inputType) { | ||
| TrackerInputType.DATE -> { | ||
| ValueType.DATE.validator.validate(value) | ||
| } | ||
| TrackerInputType.DATE_TIME -> { | ||
| ValueType.DATETIME.validator.validate(value) | ||
| } | ||
| TrackerInputType.TIME -> { | ||
| ValueType.TIME.validator.validate(value) | ||
| } | ||
|
|
||
| TrackerInputType.AGE -> { | ||
| ValueType.AGE.validator.validate(value) | ||
| } | ||
|
|
||
| else -> { | ||
| false | ||
| } |
There was a problem hiding this comment.
This implementation returns a lambda ({ when (...) { ... } }) rather than the validation result itself, so callers would receive a function object instead of a validation outcome. Also, the return type is Any, which hides the real contract (the rest of the codebase uses org.hisp.dhis.android.core.arch.helpers.Result from ValueType.*.validator.validate). Return the actual validation result (and type it explicitly) instead of a lambda/Any.
| { | |
| when (inputType) { | |
| TrackerInputType.DATE -> { | |
| ValueType.DATE.validator.validate(value) | |
| } | |
| TrackerInputType.DATE_TIME -> { | |
| ValueType.DATETIME.validator.validate(value) | |
| } | |
| TrackerInputType.TIME -> { | |
| ValueType.TIME.validator.validate(value) | |
| } | |
| TrackerInputType.AGE -> { | |
| ValueType.AGE.validator.validate(value) | |
| } | |
| else -> { | |
| false | |
| } | |
| when (inputType) { | |
| TrackerInputType.DATE -> { | |
| ValueType.DATE.validator.validate(value) | |
| } | |
| TrackerInputType.DATE_TIME -> { | |
| ValueType.DATETIME.validator.validate(value) | |
| } | |
| TrackerInputType.TIME -> { | |
| ValueType.TIME.validator.validate(value) | |
| } | |
| TrackerInputType.AGE -> { | |
| ValueType.AGE.validator.validate(value) | |
| } | |
| else -> { | |
| false |
| override suspend fun invoke(input: FetchSearchParametersData): Result<List<SearchParameterModel>> = | ||
| withContext(dispatcher.io) { | ||
| try { | ||
| val searchParameters = | ||
| input.programUid?.let { | ||
| repository.getSearchParametersByProgram(it) | ||
| } ?: repository.getSearchParametersByTrackedEntityType(input.teiTypeUid) | ||
|
|
||
| Result.success(sortSearchParameters(searchParameters)) | ||
| } catch (e: DomainError) { | ||
| Result.failure(e) | ||
| } | ||
| } | ||
|
|
||
| // Sort parameters to list first QR or BarCode uniques, then QR or BarCode and then remaining uniques | ||
| fun sortSearchParameters(parameters: List<SearchParameterModel>): List<SearchParameterModel> = | ||
| parameters.sortedWith( | ||
| compareByDescending<SearchParameterModel> { | ||
| isQrCodeOrBarCode(it.inputType) && it.isUnique | ||
| }.thenByDescending { | ||
| isQrCodeOrBarCode(it.inputType) | ||
| }.thenByDescending { it.isUnique }, | ||
| ) | ||
|
|
||
| fun isQrCodeOrBarCode(inputType: TrackerInputType?): Boolean = | ||
| inputType == TrackerInputType.QR_CODE || inputType == TrackerInputType.BAR_CODE |
There was a problem hiding this comment.
Sorting and repository-selection logic were moved here (including the QR/Barcode-first ordering), and prior tests for sorting were removed. Add unit tests for invoke selecting program vs TEI-type, and for sortSearchParameters ordering (QR/BarCode unique first, then QR/BarCode, then remaining uniques) to prevent regressions.
| SearchParameterModel( | ||
| uid = attribute.uid(), | ||
| label = attribute.displayFormName() ?: "", | ||
| inputType = | ||
| getInputType( | ||
| attribute = attribute, | ||
| isCustomIntent = customIntent != null, | ||
| valueTypeRenderingType = renderingType, | ||
| ), | ||
| optionSet = attribute.optionSet()?.uid(), | ||
| customIntentUid = customIntent?.uid, | ||
| minCharactersToSearch = attribute.minCharactersToSearch(), | ||
| searchOperator = getSearchOperator(attribute), | ||
| isUnique = isUnique(attribute.uid()), | ||
| ) |
There was a problem hiding this comment.
This code performs extra SDK lookups per attribute to compute uniqueness (isUnique(attribute.uid())), even though TrackedEntityAttribute already exposes unique(). This adds unnecessary blockingGet() calls in a tight mapping loop; prefer using attribute.unique() == true (and similarly avoid the repeated attribute lookup in the filter below if possible).
| Icon( | ||
| imageVector = Icons.Outlined.ErrorOutline, | ||
| contentDescription = "warning", | ||
| tint = AdditionalInfoItemColor.WARNING.color, |
There was a problem hiding this comment.
These contentDescription values are hard-coded English strings, so they can’t be localized (and may be redundant if the icon is purely decorative). Prefer using a stringResource for accessibility text, or set contentDescription = null when the icon should be ignored by screen readers.
| sealed interface SearchScreenUiEvent { | ||
| class OnSearchButtonClicked : SearchScreenUiEvent | ||
|
|
||
| class OnClearSearchButtonClicked : SearchScreenUiEvent | ||
|
|
||
| class OnCloseClicked : SearchScreenUiEvent |
There was a problem hiding this comment.
These event types are modeled as instantiable classes with no state, which forces allocations and requires call sites to use OnCloseClicked() etc. In this codebase, zero-arg sealed events/actions are represented as data object singletons; consider changing these to data object (or object) to match the established pattern and simplify usage.
| @Composable | ||
| private fun SearchOperator.supportingTextString() = | ||
| when (this) { | ||
| SearchOperator.EQ -> | ||
| stringResource(Res.string.equal_search_operator) | ||
| SearchOperator.SW -> | ||
| stringResource(Res.string.starts_with_search_operator) | ||
| SearchOperator.EW -> | ||
| stringResource(Res.string.end_with_search_operator) | ||
| else -> null | ||
| } |
There was a problem hiding this comment.
SearchOperator.LIKE is never mapped to supporting text (falls into else -> null), even though a like_search_operator string resource was added. Add a LIKE branch so the UI can display the intended helper text for LIKE operators.
| fun getOptionSetFlow( | ||
| fieldUid: String, | ||
| optionSetUid: String, | ||
| ): Flow<PagingData<TrackerOptionItem>> = | ||
| optionSetFlows.getOrPut(fieldUid) { | ||
| flow { |
There was a problem hiding this comment.
Option set flows are cached only by fieldUid, but this function also depends on optionSetUid. If the same fieldUid can ever be associated with a different optionSetUid (or this is called with a wrong optionSetUid), the cached flow will keep using the first optionSetUid. Key the cache by a composite (e.g., "$fieldUid:$optionSetUid") or nest maps by fieldUid -> optionSetUid.
| flow { | ||
| val searchQuery = | ||
| optionSetSearchQueries.getOrPut(fieldUid) { | ||
| MutableStateFlow(null) | ||
| } | ||
|
|
||
| searchQuery.collect { query -> | ||
| val result = | ||
| fetchOptionSetOptions( | ||
| FetchOptionSetOptions.Params( | ||
| optionSetUid = optionSetUid, | ||
| pageSize = 10, | ||
| searchQuery = query, | ||
| ), | ||
| ) | ||
|
|
||
| result.fold( | ||
| onSuccess = { optionsFlow -> | ||
| emitAll(optionsFlow) | ||
| }, | ||
| onFailure = { | ||
| emit(PagingData.empty()) | ||
| }, | ||
| ) | ||
| } | ||
| }.cachedIn(viewModelScope) |
There was a problem hiding this comment.
The flow built here collects searchQuery and then emitAll(optionsFlow) for each query. Because emitAll collects the entire upstream flow, a PagingData flow that doesn’t complete can prevent subsequent query updates from being processed. This is a good fit for flatMapLatest on the searchQuery StateFlow (or mapLatest) so each new query cancels the previous paging flow and switches to the latest one.
| flow { | |
| val searchQuery = | |
| optionSetSearchQueries.getOrPut(fieldUid) { | |
| MutableStateFlow(null) | |
| } | |
| searchQuery.collect { query -> | |
| val result = | |
| fetchOptionSetOptions( | |
| FetchOptionSetOptions.Params( | |
| optionSetUid = optionSetUid, | |
| pageSize = 10, | |
| searchQuery = query, | |
| ), | |
| ) | |
| result.fold( | |
| onSuccess = { optionsFlow -> | |
| emitAll(optionsFlow) | |
| }, | |
| onFailure = { | |
| emit(PagingData.empty()) | |
| }, | |
| ) | |
| } | |
| }.cachedIn(viewModelScope) | |
| val searchQuery = | |
| optionSetSearchQueries.getOrPut(fieldUid) { | |
| MutableStateFlow<String?>(null) | |
| } | |
| searchQuery | |
| .flatMapLatest { query -> | |
| flow { | |
| val result = | |
| fetchOptionSetOptions( | |
| FetchOptionSetOptions.Params( | |
| optionSetUid = optionSetUid, | |
| pageSize = 10, | |
| searchQuery = query, | |
| ), | |
| ) | |
| result.fold( | |
| onSuccess = { optionsFlow -> | |
| emitAll(optionsFlow) | |
| }, | |
| onFailure = { | |
| emit(PagingData.empty()) | |
| }, | |
| ) | |
| } | |
| } | |
| .cachedIn(viewModelScope) |
| class SearchParametersRepositoryImpl : SearchParametersRepository { | ||
| override suspend fun getSearchParametersByProgram(programUid: String): List<SearchParameterModel> { | ||
| TODO("Not yet implemented") | ||
| } | ||
|
|
||
| override suspend fun getSearchParametersByTrackedEntityType(trackedEntityTypeUid: String): List<SearchParameterModel> { | ||
| TODO("Not yet implemented") | ||
| } |
There was a problem hiding this comment.
This desktop implementation is left as TODO() for both repository methods, which will crash at runtime if any desktop code path requests search parameters. Either provide a safe stub (e.g., emptyList()) or throw a clear, non-TODO exception indicating the feature is unsupported on desktop.
3 participants
No description provided.