Include min and max values in the validation message in a number question if the values are in the extension#2763
Include min and max values in the validation message in a number question if the values are in the extension#2763chungwwei wants to merge 9 commits intogoogle:masterfrom
Conversation
…tion if the values are in the extension
...java/com/google/android/fhir/datacapture/views/factories/EditTextIntegerViewHolderFactory.kt
Outdated
Show resolved
Hide resolved
|
|
||
| val minValue = | ||
| (questionnaireViewItem.minAnswerValue as? IntegerType)?.value ?: Int.MIN_VALUE | ||
| val maxValue = | ||
| (questionnaireViewItem.maxAnswerValue as? IntegerType)?.value ?: Int.MAX_VALUE | ||
|
|
There was a problem hiding this comment.
what happens when the minValue is bigger than the maxValue?
how about we throw an exception when that happens?
There was a problem hiding this comment.
I added the exception. UI will crash when minValue > maxValue, like the slider and date widgets.
There was a problem hiding this comment.
Okay, but feel free to speak your thoughts on this, we could for example: show an error message to the user, instead of outright crashing the app. We're open for discussion.
What do you think?
There was a problem hiding this comment.
Ig it's rare for a scenario like that to happen. Adheres to the rules, but with nonsensical values.
Alternatively, I view the minValue > maxValue a meaningless constraint on the UI, so it make sense to treat it as though the constraint doesn’t exist? We can set minAnswerValue and maxAnswerValue to null if when that happens.
Looks like the web app https://lhcforms.nlm.nih.gov/lhcforms ignored that scenario
jingtang10
left a comment
There was a problem hiding this comment.
Thanks for adding this. I'm ok with this PR.
Fikri's point is a good one - we do throw exceptions in other widgets when min value > max value. But I'm a bit concerned about the UI crasing... I wonder if we actually should remove that exception from the date and slider widgets.
But I'm open-minded in this case. I'll approve this PR - but pls resolve the exception question with fikri - and we can then merge this pr.
Thanks again for this - good work!
|
|
||
| override fun bind(questionnaireViewItem: QuestionnaireViewItem) { | ||
| super.bind(questionnaireViewItem) | ||
|
|
||
| val minValue = (questionnaireViewItem.minAnswerValue as? IntegerType)?.value | ||
| val maxValue = (questionnaireViewItem.maxAnswerValue as? IntegerType)?.value | ||
|
|
||
| if (minValue != null && maxValue != null && minValue > maxValue) { | ||
| throw IllegalArgumentException("minValue cannot be greater than maxValue") | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
I feel overriding the bind function is not necessary, updateValidationTextUI function is already called in the parent's bind (you can check it), so throwing the error in updateValidationTextUI should suffice.
What do you think?
There was a problem hiding this comment.
I had it that way initially, but i thought placing the constraint check didn’t fit the intent of the function name updateValidationTextUI, so I moved it to bind. Like jingtang10 commented, it would be better to have it in the view model to remove redundancy
maybe have it in QuestionnaireItemViewHolder, I believe all the factories are extending QuestionnaireItemViewHolder
|
if you have the max and min values on a different question type, e.g. slider? or date / date time? if you put this logic in questionnaire view model this would be done for all widgets - instead of specific ui widgets. |
|
hey @jingtang10, thanks for the pointer! I made some changes to the code based on your last comment. could you please take a look and let me know what you think |
IMPORTANT: All PRs must be linked to an issue (except for extremely trivial and straightforward changes).
Fixes #2757
Description
Show
number must be between <INT_MIN> and <INT_MAX>when input is not an integer.Use the max and min values in the extension if they are defined.
Alternative(s) considered
Have you considered any alternatives? And if so, why have you chosen the approach in this PR?
Type
Choose one: Bug fix
Screenshots (if applicable)
Screen_recording_20250101_225054.webm
Checklist
./gradlew spotlessApplyand./gradlew spotlessCheckto check my code follows the style guide of this project../gradlew checkand./gradlew connectedCheckto test my changes locally.