-
Notifications
You must be signed in to change notification settings - Fork 35
feat: [DHIS2-20429][DHIS2-20530] use configured TEA Search Operators in the Search forms #4413
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
🚀 Deployed on https://deploy-preview-4413.capture.netlify.dhis2.org |
...es/capture-core/metaDataStoreLoaders/programs/quickStoreOperations/storeProgramIndicators.ts
Outdated
Show resolved
Hide resolved
JoakimSM
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a good start @simonadomnisoru
| set preferredSearchOperator(preferredSearchOperator: string | undefined) { | ||
| this._preferredSearchOperator = preferredSearchOperator; | ||
| } | ||
| get preferredSearchOperator(): string | undefined { | ||
| return this._preferredSearchOperator; | ||
| } | ||
|
|
||
| set blockedSearchOperators(blockedSearchOperators: Array<string> | undefined) { | ||
| this._blockedSearchOperators = blockedSearchOperators; | ||
| } | ||
| get blockedSearchOperators(): Array<string> | undefined { | ||
| return this._blockedSearchOperators; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need these new properties on the DataElement class here, or is the searchOperator enough?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great question, I have the same doubt. I think searchOperator will be enough. I'll remove the other two. Thank you for the feedback!
henrikmv
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work, Simona! I did an initial review and left some comments. Thank you!
src/core_modules/capture-core/components/FormFields/New/HOC/messages/withSearchHelpMessage.tsx
Outdated
Show resolved
Hide resolved
...taDataMemoryStoreBuilders/common/factory/searchGroup/searchOperator/forcedSearchOperators.ts
Outdated
Show resolved
Hide resolved
| {helpText} <br /> | ||
| </div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’d recommend not showing the help text for TEAs with optionSet configured or value types Yes/No and Yes Only, as users can only select from predefined options and the help text doesn’t add value - just extra noise in the form. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great suggestion @henrikmv,
I agree that the help text below Yes/No (BOOLEAN) and Yes Only (TRUE_ONLY) doesn’t add much value.

I’ve removed it, which makes the form cleaner 👍
On the other hand, for the help text below the optionSet, I do think it adds value. Option sets behave differently for the MULTI_TEXT value type (the preferredSearchOperator is applied in this case). Since users are not aware of this difference and all optionSet use the same dropdown selector, I think it could be confusing to show the help text only for some dropdowns. WDYT?
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the insight @simonadomnisoru. I am a bit concerned whether the preferredSearchOperator should be applied to MULTI_TEXT option sets.
For MULTI_TEXT option sets, searches can only be performed using a single value, and the order of the values being searched through is not taken into account. As a result, the selected preferred search operator does not influence the search outcome; the behaviour remains the same regardless of the operator chosen.
WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This goes a bit beyond the MULTI_TEXT discussion, but I wanted to share something for consideration.
It seems to me that preferredSearchOperator may not have a practical impact for some value types. For example:
AGE— always matched on an exact dateMULTI_TEXT— always matched on full valueUSERNAME— always matched on the full usernameORGANISATION_UNIT— always matched on the full organisation unit name
In addition, I’m not entirely sure why PERCENTAGE is not handled as a range-based value type? Not related to your changes, but related to the feature itself.
From my perspective, preferredSearchOperator seems useful for value types that are not range-based, and where search can be performed on partial values:
TEXTLONG_TEXTEMAILPHONE_NUMBER
I would be really happy to be corrected if I’m missing something here - I just wanted to raise this as a point for discussion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @ameenhere, what do you think about the concerns raised by @henrikmv? I’ll summarize below what we agreed to implement in the first iteration of the search improvement initiative:
-
Value types that currently use
EQwill ignore thepreferredSearchOperatorand continue to useEQ. These are:- BOOLEAN
- TRUE_ONLY
- AGE
-
Value types that currently use the
To-Fromrange will ignore thepreferredSearchOperatorand continue to useGEandLE. These are:- NUMBER
- INTEGER
- INTEGER_POSITIVE
- INTEGER_NEGATIVE
- INTEGER_ZERO_OR_POSITIVE
- DATE
- DATETIME
- TIME
-
Value types that currently use
LIKEwill start using thepreferredSearchOperatorinstead. These are:- TEXT
- LONG_TEXT
- PHONE_NUMBER
- PERCENTAGE
- MULTI_TEXT
- ORGANISATION_UNIT
- USERNAME
The main point of the first iteration of search improvements was simply to offer an alternative to LIKE for the value types mentioned in point 3. It’s now up to the implementer/admin to decide which preferredSearchOperator has a practical impact for those value types. Does it make sense, in this first iteration, to further reduce the list to only the value types mentioned by @henrikmv: TEXT, LONG_TEXT, EMAIL, and PHONE_NUMBER?
@henrikmv Regarding PERCENTAGE, I don't know why isn’t handled as a range-based value type. If we want to change that, it sounds like a separate ticket to me
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @simonadomnisoru
- I think documenting the fallback cases is also a good addition. We will also need to document this behaviour for the end user somewhere as well.
- If LIKE is blocked for MULTI_TEXT, wouldnt Capture "fallback" to SW or EQ like others?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I documented the fallback cases in Confluence too. Sure, the feature will have its behavior documented for the end user in the Capture app docs
- Yes, it could fall back to SW or EQ like the others, but above you mentioned that "LIKE is the only operator usable for a MULTI_TEXT value type". So if that is the case, I’m confused why we would allow it to be blocked if it is the only one that works
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah. To clarify, the operation will go through with SW and EQ as well. The results will not be what we expect it to be.
Anyway.. let us move that to a separate ticket (either BE, or FE change, or both).
But we can go ahead with the changes in this PR 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @simonadomnisoru ! I played around a bit with the changes on Friday, and I think I agree with @henrikmv even more in terms of the help text.
In the Capture UI, for username, orgUnit and optionset, we always show a drop down. Meaning there is no possibility in the Capture UI to for user to type a partial text for a username or orgunit or any optionset. So that already gives an impression that the search is going to be attempted with an exact match.
However on discussing with you, I understand that Capture is using LIKE operator, even though the UI feels like it is using an exact match.
So my thinking currently is
For OrgUnit and Username valueType. We switch to EQ and ignore the preferredSearchOperator. (Similar to OptionSet)
In addition to the above, we do not show any help text for these valueTypes, since the UI already indicates it is a direct match by selecting an option. Wdyt @simonadomnisoru ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to me, thank you for your input @ameenhere. I’ve updated the code to ignore preferredSearchOperator for USERNAME and ORGANISATION_UNIT. I’ve also documented our latest agreements on how Capture computes the search operator for each Value type in Confluence.
I created a bug for the MULTI_TEXT issue -> https://dhis2.atlassian.net/browse/DHIS2-20694
Let me know if it looks good now @henrikmv and @ameenhere
|



DHIS2-20429 and DHIS2-20530
Tech summary:
The search operator is computed like this:
1) Value types that always use
EQThe following value types will ignore
preferredSearchOperatorand always use theEQoperator:BOOLEAN,TRUE_ONLY,AGE,ORGANISATION_UNIT,USERNAME, Unique TEAs, and TEAs with option sets (exceptMULTI_TEXT).2) Value types that currently use a from–to range
These will ignore
preferredSearchOperatorand continue usingGEandLE. The following value types fall into this category:NUMBER,INTEGER,INTEGER_POSITIVE,INTEGER_NEGATIVE,INTEGER_ZERO_OR_POSITIVE,DATE,DATETIME, andTIME.3) Value types that currently use LIKE
These will start using the
preferredSearchOperatormetadata. IfpreferredSearchOperatoris not defined, they will fall back to the first available operator in[LIKE, SW, EQ]that is not listed inblockedSearchOperators. The following value types fall into this category:TEXT,LONG_TEXT,EMAIL,PHONE_NUMBER, andPERCENTAGE.4)
MULTI_TEXTvalue typeIt will ignore
preferredSearchOperatorand fall back to the first available operator in[LIKE, SW, EQ]that is not listed inblockedSearchOperators.