-
Notifications
You must be signed in to change notification settings - Fork 299
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
Constraint support (local and error case only) #2199
Conversation
459317c
to
7abd642
Compare
@jingtang10 @santosh-pingle |
@shelaghm we might need a new icon in the catalog app for this new behavior |
@FikriMilano How about this function icon? I recommend just pulling from this list of open source icons if you ever need one since these are easy to implement and open source. |
@shelaghm That's fine, thanks! |
@shelaghm Done. |
@FikriMilano Could you make the icon slightly smaller: 48px? Thanks! |
not sure i am super keen on the sigma sign - it means sum in mathamatics so i think it's a bit weird to use that for constraint. how about this: https://fonts.google.com/icons?selected=Material+Symbols+Outlined:rule:FILL@0;wght@400;GRAD@0;opsz@24&icon.query=rule? |
@FikriMilano how are you determining where to show the error message since constraints are global? |
"Rule' one is better, thanks @jingtang10 @FikriMilano |
@jingtang10 I kind of forgot that the extension can be applied to the Questionnaire / globally, so I haven't add the code to do that yet. The solution would be: use a sub-extension called or we can use a snackbar/toast/modal to show an error message if the constraint extension is declared in the Questionnaire itself / globally. |
i think in the global case, i'm happy for it to be displayed on top of the questionnaire or in the confirmation popup dialog before submission. i feel deduction of where the error message should be could end up being unreliable / not what the questionnaire author expects.
|
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 @FikriMilano!
I see that you have created another validator that takes in some addition contextual data. Wouldn't another approach be that we pass the questionnaire response and expression evaluator and context to the validator itself upon constrution so to make the evaluation of any expression contextual within a particular questionnaire?
That way, i think you don't really need to create a new interface? as the interface would just be the same as the QuestionnaireResponseItemConstraintValidator
?
Have you considered this? Can you briefly analyse the pros and cons of this approach? Thanks!
For this 'local case' it's probably a good idea to refactor this to use the FHIRPath supplement It might be cleaner and more maintainable maybe? |
Additionally perhaps as part of this PR we should enforce that |
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.
Left some comments, in general I was wondering if we could re-use the existing interface.
It is also not clear as to what changes will need to be done to support global mode of this extension. It might be worthwhile adding a TODO with a small explanation of what changes will be needed with an issue link.
datacapture/src/main/java/com/google/android/fhir/datacapture/QuestionnaireViewModel.kt
Outdated
Show resolved
Hide resolved
.../main/java/com/google/android/fhir/datacapture/extensions/MoreQuestionnaireItemComponents.kt
Show resolved
Hide resolved
...ain/java/com/google/android/fhir/datacapture/validation/MultiExtensionConstraintValidator.kt
Outdated
Show resolved
Hide resolved
...ure/src/main/java/com/google/android/fhir/datacapture/validation/MultiConstraintValidator.kt
Outdated
Show resolved
Hide resolved
...in/java/com/google/android/fhir/datacapture/validation/QuestionnaireResponseItemValidator.kt
Outdated
Show resolved
Hide resolved
...ure/src/main/java/com/google/android/fhir/datacapture/validation/MultiConstraintValidator.kt
Outdated
Show resolved
Hide resolved
...ure/src/main/java/com/google/android/fhir/datacapture/validation/MultiConstraintValidator.kt
Outdated
Show resolved
Hide resolved
da8cf0e
to
a3497d1
Compare
a3497d1
to
6f7694b
Compare
@jingtang10 @ellykits @joiskash |
...src/main/java/com/google/android/fhir/datacapture/validation/ConstraintExtensionValidator.kt
Outdated
Show resolved
Hide resolved
...src/main/java/com/google/android/fhir/datacapture/validation/ConstraintExtensionValidator.kt
Outdated
Show resolved
Hide resolved
...src/main/java/com/google/android/fhir/datacapture/validation/ConstraintExtensionValidator.kt
Outdated
Show resolved
Hide resolved
...src/main/java/com/google/android/fhir/datacapture/validation/ConstraintExtensionValidator.kt
Outdated
Show resolved
Hide resolved
...m/google/android/fhir/datacapture/validation/QuestionnaireResponseItemConstraintValidator.kt
Show resolved
Hide resolved
…stionnaire-constraint
…droid-fhir into 2003-questionnaire-constraint
@MJ1998 this is ready for review |
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.
LGTM
@MJ1998 @jingtang10 The cons is: So I ended up taking a similar approach: The changes are:
Before my change:
After my change:
|
IMPORTANT: All PRs must be linked to an issue (except for extremely trivial and straightforward changes).
Fixes #1849
Description
Alternative(s) considered
N/A
Type
Feature
Demo Video
Screen_recording_20230927_111829.webm
Catalog Behavior Image
@shelaghm
Checklist
./gradlew spotlessApply
and./gradlew spotlessCheck
to check my code follows the style guide of this project../gradlew check
and./gradlew connectedCheck
to test my changes locally.