Add ability to validate the min/max for Date on a FHIR Questionnaire #1050

RaaziaTarique · 2022-01-18T14:48:38Z

IMPORTANT: All PRs must be linked to an issue (except for extremely trivial and straightforward changes).

Description
Clear and concise code change description.

Alternative(s) considered
Have you considered any alternatives? And if so, why have you chosen the approach in this PR?

Type
Choose one: Feature

Screenshots (if applicable)

Checklist

I have read and acknowledged the Code of conduct
I have read How to Contribute
I have read the Developer's guide
I have signed the Google Individual CLA, or I am covered by my company's Corporate CLA
I have discussed my proposed solution with code owners in the linked issue(s) and we have agreed upon the general approach
I have run ./gradlew spotlessApply and ./gradlew spotlessCheck to check my code follows the style guide of this project
I have run ./gradlew check and ./gradlew connectedCheck to test my changes locally
I have built and run the reference app(s) to verify my change fixes the issue and/or does not break the reference app(s)

… current date. - Add test case in QuestionnaireItemDatePickerViewHolderFactoryInstrumentedTest for checking "today()" filter code

codecov · 2022-01-18T15:47:12Z

Codecov Report

Merging #1050 (a0d471a) into master (a292e50) will increase coverage by 0.00%.
The diff coverage is 88.23%.

@@            Coverage Diff            @@
##             master    #1050   +/-   ##
=========================================
  Coverage     84.15%   84.15%           
  Complexity      666      666           
=========================================
  Files           146      146           
  Lines         10609    10622   +13     
  Branches        807      809    +2     
=========================================
+ Hits           8928     8939   +11     
  Misses         1278     1278           
- Partials        403      405    +2

Impacted Files	Coverage Δ
...d/fhir/datacapture/utilities/TypeConversionUtil.kt	`84.61% <77.77%> (-15.39%)`	⬇️
...acapture/validation/MaxValueConstraintValidator.kt	`100.00% <100.00%> (ø)`
...acapture/validation/MinValueConstraintValidator.kt	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 92cceb7...a0d471a. Read the comment docs.

…y()" filter code

…date_validation

- Add test case in MinValueConstraintValidatorTest and QuestionnaireItemDatePickerViewHolderFactoryInstrumentedTest for checking "today()" filter code

…intValidator.kt

…seItemValidatorTest

…raintValidatorTest.kt and MinValueConstraintValidatorTest.kt

jingtang10 · 2022-01-25T14:00:56Z

Hi @RaaziaTarique! Thanks for the PR. If this is ready for review can you mark it so?

RaaziaTarique · 2022-01-25T14:03:53Z

Hi @RaaziaTarique! Thanks for the PR. If this is ready for review can you mark it so?

Hi @jingtang10, this PR is ready but currently code coverage check for patch and project is failing. That's why it's still marked as draft.

jingtang10

@RaaziaTarique thanks for this PR!

Can you please point me to any documentation (on hl7.org or other sites) on the expression today()? I've seen this expression in questionnaires but couldn't find any actual documentation in the specifications.

jingtang10 · 2022-02-09T09:13:01Z

.../src/main/java/com/google/android/fhir/datacapture/validation/MaxValueConstraintValidator.kt

+        if (extension.value is StringType && (extension.value as StringType).value.equals("today()")
+        ) {
+          DateType(Date())
+        } else {
+          extension.value
+        }


what you have done here, is to add the capability to parse an expression and convert it into a value of a different type. This shouldn't be done inside this class. There should be another class that handles the parsing and interpreting of expressions.

likewise for the min value constraint validator. you should not have the code duplication in these 2 classes.

I'll create another class and will do the parsing there and use it on both max and min validator classes.

jingtang10 · 2022-02-09T09:14:49Z

.../test/java/com/google/android/fhir/datacapture/validation/MaxValueConstraintValidatorTest.kt

+      QuestionnaireResponse.QuestionnaireResponseItemComponent().apply {
+        addAnswer(
+          QuestionnaireResponse.QuestionnaireResponseItemAnswerComponent().apply {
+            value = DateType(2026, 0, 1)


so this test will start failing in year 2026?

is there a way for you to mock/fake the system date to make this test more stable.

Instead of using hardcoded date I can pass 5 years future date from current date. This will stable this test case as well.

…date_validation

# Conflicts: # datacapture/src/androidTest/java/com/google/android/fhir/datacapture/views/QuestionnaireItemDatePickerViewHolderFactoryInstrumentedTest.kt

RaaziaTarique · 2022-02-09T11:16:35Z

today()

I just found this sample "Library" where they using something similar to it http://hl7.org/fhir/uv/cpg/Library-CKDRiskLogic.html. Maybe @f-odhiambo can comment better on it.

f-odhiambo · 2022-02-09T12:01:19Z

today()

I just found this sample "Library" where they using something similar to it http://hl7.org/fhir/uv/cpg/Library-CKDRiskLogic.html. Maybe @f-odhiambo can comment better on it.

Looking into this :)

RaaziaTarique · 2022-02-09T12:03:11Z

today()

I just found this sample "Library" where they using something similar to it http://hl7.org/fhir/uv/cpg/Library-CKDRiskLogic.html. Maybe @f-odhiambo can comment better on it.

Found the link for this particular filter. http://hl7.org/fhirpath/#datetime-arithmetic CC: @jingtang10 @f-odhiambo

jingtang10 · 2022-02-23T20:19:47Z

@RaaziaTarique have you used FHIRPathEngine before? See our usage https://github.com/google/android-fhir/search?q=fhirpathengine.

Can you try to use that instead of manually handling expressions?

RaaziaTarique · 2022-02-25T07:18:04Z

@RaaziaTarique have you used FHIRPathEngine before? See our usage https://github.com/google/android-fhir/search?q=fhirpathengine.

Can you try to use that instead of manually handling expressions?

@jingtang10, I have not used it previously, checking it now.

…date_validation

RaaziaTarique · 2022-03-02T13:02:45Z

@RaaziaTarique have you used FHIRPathEngine before? See our usage https://github.com/google/android-fhir/search?q=fhirpathengine.
Can you try to use that instead of manually handling expressions?

@jingtang10, I have not used it previously, checking it now.

@jingtang10 , I checked that ResourceMapper is using FHIRPathEngine but currently it's only using (http://hl7.org/fhir/uv/sdc/StructureDefinition/sdc-questionnaire-initialExpression)

In ResourceMapper I added the functionality to detect "today()" field if passed as an expression and in ResourceMapper we are not using http://hl7.org/fhir/StructureDefinition/maxValue and http://hl7.org/fhir/StructureDefinition/minValue expressions as these expressions are being validated separately.

…her issue #1033

jingtang10

@maimoonak @RaaziaTarique @ekigamba please take a look i think this PR's logic is still incorrect.

jingtang10 · 2022-05-30T20:34:54Z

.../src/main/java/com/google/android/fhir/datacapture/validation/MaxValueConstraintValidator.kt

@@ -29,10 +31,13 @@ internal object MaxValueConstraintValidator :
    predicate = {
      extension: Extension,
      answer: QuestionnaireResponse.QuestionnaireResponseItemAnswerComponent ->
-      answer.value > extension.value
+      answer.value > extension.value.detectTodayDate()


so this is assuming if there's a max value constraint for a date it will be today's date? this is incorrect logic!

jingtang10 · 2022-05-30T20:35:06Z

datacapture/src/main/java/com/google/android/fhir/datacapture/utilities/TypeConversionUtil.kt

+internal fun Type.detectTodayDate(): Type =
+  if (this is StringType && value.equals("today()")) {
+    DateType(Date())
+  } else {
+    this
+  }


this function signature is not great - it's very confusing. please refactor!

you shouldn't have vastly different behaviors due to date time.

jingtang10 · 2022-06-04T16:19:27Z

@maimoonak @shoaibmushtaq25 @f-odhiambo can someone take over this PR please?

My main issue with this PR is that the logic is very specific - we can't have this type of hard-coded logic just for today. We need to make a change that handles expressions in general. I'm ok with not having a full implementation of that, but the structure needs to be there. At the moment the code logic seems strange.

Tarun-Bhardwaj · 2022-06-15T11:58:10Z

@maimoonak @shoaibmushtaq25 @f-odhiambo can someone take over this PR please?

My main issue with this PR is that the logic is very specific - we can't have this type of hard-coded logic just for today. We need to make a change that handles expressions in general. I'm ok with not having a full implementation of that, but the structure needs to be there. At the moment the code logic seems strange.

@maimoonak @shoaibmushtaq25 @f-odhiambo is someone working on the changes suggested by @jingtang10 above?

f-odhiambo · 2022-06-15T13:08:35Z

@maimoonak @shoaibmushtaq25 @f-odhiambo can someone take over this PR please?
My main issue with this PR is that the logic is very specific - we can't have this type of hard-coded logic just for today. We need to make a change that handles expressions in general. I'm ok with not having a full implementation of that, but the structure needs to be there. At the moment the code logic seems strange.

@maimoonak @shoaibmushtaq25 @f-odhiambo is someone working on the changes suggested by @jingtang10 above?

@aurangzaibumer is working on this and should be raising a PR. You can reassign to him @Tarun-Bhardwaj

jingtang10 · 2022-06-15T19:06:53Z

ok closing this in that case. thanks @f-odhiambo and @aurangzaibumer.

RaaziaTarique added 2 commits January 18, 2022 14:57

- Update MaxValueConstraintValidator to validate "today()" filter for…

c171125

… current date. - Add test case in QuestionnaireItemDatePickerViewHolderFactoryInstrumentedTest for checking "today()" filter code

Merge branch 'master' into 1040_date_validation

9a8396f

RaaziaTarique added 5 commits January 18, 2022 22:30

- Add test case in MaxValueConstraintValidatorTest for checking "toda…

fe649e4

…y()" filter code

Merge remote-tracking branch 'origin/1040_date_validation' into 1040_…

b317926

…date_validation

- Update MinValueConstraintValidator for checking "today()" filter

0ef39c2

- Add test case in MinValueConstraintValidatorTest and QuestionnaireItemDatePickerViewHolderFactoryInstrumentedTest for checking "today()" filter code

- Simplify code of MaxValueConstraintValidator.kt and MinValueConstra…

c94d5ec

…intValidator.kt

- Test case added for min-max "today()" filter in QuestionnaireRespon…

b85de33

…seItemValidatorTest

RaaziaTarique self-assigned this Jan 24, 2022

- Positive flows tested for min-max "today()" filter in MaxValueConst…

38dc015

…raintValidatorTest.kt and MinValueConstraintValidatorTest.kt

RaaziaTarique added 2 commits January 25, 2022 15:07

Merge branch 'master' into 1040_date_validation

4243ab7

Merge branch 'master' into 1040_date_validation

217facb

RaaziaTarique marked this pull request as ready for review January 27, 2022 07:04

RaaziaTarique requested review from aditya-07, deepankarb, jingtang10, joiskash, kevinmost, stevenckngaa and yigit as code owners January 27, 2022 07:04

RaaziaTarique added 2 commits January 27, 2022 12:36

Merge branch 'master' into 1040_date_validation

99c86ec

Merge branch 'master' into 1040_date_validation

4c0bd82

jingtang10 requested changes Feb 9, 2022

View reviewed changes

RaaziaTarique added 3 commits February 9, 2022 10:53

Merge remote-tracking branch 'origin/1040_date_validation' into 1040_…

5aca35f

…date_validation

- Code revised as per jing's comments

4775942

Merge branch 'master' into 1040_date_validation

85f139b

# Conflicts: # datacapture/src/androidTest/java/com/google/android/fhir/datacapture/views/QuestionnaireItemDatePickerViewHolderFactoryInstrumentedTest.kt

RaaziaTarique requested a review from jingtang10 February 9, 2022 11:06

RaaziaTarique added 2 commits February 10, 2022 09:41

Merge branch 'master' into 1040_date_validation

e1d39ef

Merge branch 'master' into 1040_date_validation

694e0dc

Merge branch 'master' into 1040_date_validation

1d191e4

RaaziaTarique requested review from ktarasenko and vvikramraj as code owners March 1, 2022 16:06

RaaziaTarique added 2 commits March 1, 2022 17:27

Merge remote-tracking branch 'origin/1040_date_validation' into 1040_…

701075e

…date_validation

Merge branch 'master' into 1040_date_validation

dea2b27

RaaziaTarique added 6 commits March 2, 2022 14:13

- today() expression functionality added in ResourceMapper.kt

db19e45

Merge branch 'master' into 1040_date_validation

848a7ab

Merge branch 'master' into 1040_date_validation

ed6343f

Merge branch 'master' into 1040_date_validation

fbac673

- Remove today() recognition from ResourceMapper as it's part of anot…

c96c76b

…her issue #1033

Merge branch 'master' into 1040_date_validation

a0d471a

jingtang10 requested changes May 30, 2022

View reviewed changes

jingtang10 closed this Jun 15, 2022

jingtang10 deleted the 1040_date_validation branch July 1, 2022 17:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ability to validate the min/max for Date on a FHIR Questionnaire #1050

Add ability to validate the min/max for Date on a FHIR Questionnaire #1050

RaaziaTarique commented Jan 18, 2022 •

edited

Loading

codecov bot commented Jan 18, 2022 •

edited

Loading

jingtang10 commented Jan 25, 2022

RaaziaTarique commented Jan 25, 2022 •

edited

Loading

jingtang10 left a comment

jingtang10 Feb 9, 2022

RaaziaTarique Feb 9, 2022

jingtang10 Feb 9, 2022

RaaziaTarique Feb 9, 2022

RaaziaTarique commented Feb 9, 2022

f-odhiambo commented Feb 9, 2022

RaaziaTarique commented Feb 9, 2022

jingtang10 commented Feb 23, 2022

RaaziaTarique commented Feb 25, 2022

RaaziaTarique commented Mar 2, 2022

jingtang10 left a comment

jingtang10 May 30, 2022

jingtang10 May 30, 2022

jingtang10 commented Jun 4, 2022

Tarun-Bhardwaj commented Jun 15, 2022

f-odhiambo commented Jun 15, 2022

jingtang10 commented Jun 15, 2022

Add ability to validate the min/max for Date on a FHIR Questionnaire #1050

Add ability to validate the min/max for Date on a FHIR Questionnaire #1050

Conversation

RaaziaTarique commented Jan 18, 2022 • edited Loading

codecov bot commented Jan 18, 2022 • edited Loading

Codecov Report

jingtang10 commented Jan 25, 2022

RaaziaTarique commented Jan 25, 2022 • edited Loading

jingtang10 left a comment

Choose a reason for hiding this comment

jingtang10 Feb 9, 2022

Choose a reason for hiding this comment

RaaziaTarique Feb 9, 2022

Choose a reason for hiding this comment

jingtang10 Feb 9, 2022

Choose a reason for hiding this comment

RaaziaTarique Feb 9, 2022

Choose a reason for hiding this comment

RaaziaTarique commented Feb 9, 2022

f-odhiambo commented Feb 9, 2022

RaaziaTarique commented Feb 9, 2022

jingtang10 commented Feb 23, 2022

RaaziaTarique commented Feb 25, 2022

RaaziaTarique commented Mar 2, 2022

jingtang10 left a comment

Choose a reason for hiding this comment

jingtang10 May 30, 2022

Choose a reason for hiding this comment

jingtang10 May 30, 2022

Choose a reason for hiding this comment

jingtang10 commented Jun 4, 2022

Tarun-Bhardwaj commented Jun 15, 2022

f-odhiambo commented Jun 15, 2022

jingtang10 commented Jun 15, 2022

RaaziaTarique commented Jan 18, 2022 •

edited

Loading

codecov bot commented Jan 18, 2022 •

edited

Loading

RaaziaTarique commented Jan 25, 2022 •

edited

Loading