Change validateQuestionnaireResponseItems to allow questionnaire response with missing items #926

joiskash · 2021-11-17T10:03:12Z

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

Description
Re-wrote validateQuestionnaireResponseItems to take the input questionnaire response and validate it against the questionnaire response that is created by using the questionnaire.

Alternative(s) considered
Yes the alternative was to use the input questionnaire response as the main questionnaire response and add items if and when enable was activated. This seemed to be a more complicated solution

Type
Bug fix

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)

…ionnaire response created by traversing through the questionnaire

… items 2. Input questionnaire Response has less items

…re items than the questionnaire

joiskash · 2021-11-17T10:11:44Z

@jingtang10 please take a look at this. These two questions are dealt as follows:

How do we deal with a questionnaire response that has more items than the questionnaire? Currently throwing an error No matching questionnaire item for questionnaire response item
How do we deal with a questionnaire response that has less items than the questionnaire? Not throwing any error because questionnaire response may not have disabled items.

…hub.com/google/android-fhir into kj/validation-fix-questionnaireResponse

codecov · 2021-11-18T09:39:29Z

Codecov Report

Merging #926 (2a6bf28) into master (9b53185) will decrease coverage by 0.00%.
The diff coverage is 94.82%.

@@             Coverage Diff              @@
##             master     #926      +/-   ##
============================================
- Coverage     88.27%   88.27%   -0.01%     
- Complexity      496      500       +4     
============================================
  Files           113      113              
  Lines          9365     9390      +25     
  Branches        649      654       +5     
============================================
+ Hits           8267     8289      +22     
- Misses          750      751       +1     
- Partials        348      350       +2

Impacted Files	Coverage Δ
...android/fhir/datacapture/QuestionnaireViewModel.kt	`81.46% <90.00%> (-1.96%)`	⬇️
...pture/validation/QuestionnaireResponseValidator.kt	`95.23% <95.83%> (+1.12%)`	⬆️
...va/com/google/android/fhir/db/impl/DatabaseImpl.kt	`88.88% <0.00%> (-1.24%)`	⬇️

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 9b53185...2a6bf28. Read the comment docs.

…or primitives

jingtang10

Some high-level comments:

Should the validation code live inQuestionnaireResponseValidator.kt instead?
I think there are two things being done here in the validation code in the view model, one is to validate that the questionnaire response matches the questionnaire, and two is create the matching questionnaire response item for each questionnaire item. I think it probably would be more useful to separate these two processes.
Given that we are now allowing for missing questionnaire response items, should we allow for that? This would mean that when we're generating the view items in the recycler view, we might have no response item in the tuple.

joiskash · 2021-11-23T15:17:04Z

Should the validation code live in QuestionnaireResponseValidator.kt instead?
That class is responsible for validating answers that are typed in via UI. I dont think we should mix them up. Maybe we change the name of this function to checkQuestionnaireResponseAgainstQuestionnaire

This function is independent of creating the questionnaire response item . What I have done is to use the questionnaire response item that is created with the questionnaire and only fill up answers from the input questionnaire response after verifying that

linkIDs match and order of questions is maintained
type matches for the primitive types
only questionnaire items which allow repeats allow multiple answers
There are no extra questionnaire response items

Since we are not using the input questionnaire response as the main questionnaire response to build view items, we will not be generating view items without response in the tuple. This is to make sure that irrespective of the items present in the input questionnaire response, the SDK will allow the user to answer the questionnaire completely.

PS: input questionnaire response is the one that is passed to the fragment via the bundle. The main questionnaire response is what we create by going through the questionnaire hierarchy.

… removed the need to use questionnaire Response for validation.

…Response

…tionnaireItem.type

jingtang10 · 2021-12-08T20:26:57Z

@kevinmost can you please also take a look at this? thanks!

kevinmost · 2021-12-09T16:45:51Z

datacapture/src/main/java/com/google/android/fhir/datacapture/QuestionnaireViewModel.kt

+          // If there is an enabled questionnaire response available then we use that. Or else we
+          // just use an empty questionnaireResponse Item
+          if (responseIndex < questionnaireResponseItemList.size &&
+              questionnaireItem.linkId.equals(questionnaireResponseItem.linkId)


Nit: Prefer == in Kotlin

kevinmost · 2021-12-09T16:46:38Z

datacapture/src/main/java/com/google/android/fhir/datacapture/QuestionnaireViewModel.kt

@@ -248,14 +249,22 @@ internal class QuestionnaireViewModel(application: Application, state: SavedStat
    pagination: QuestionnairePagination?,
  ): QuestionnaireState {
    // TODO(kmost): validate pages before switching between next/prev pages
+    var responseIndex = 0
    val items: List<QuestionnaireItemViewItem> =
      questionnaireItemList
        .asSequence()
        .withIndex()


Nit: Would be a bit more performant to use

.flatMapIndexed { index, questionnaireItem ->

kevinmost · 2021-12-09T16:54:59Z

...c/main/java/com/google/android/fhir/datacapture/validation/QuestionnaireResponseValidator.kt

+    questionnaireResponseInputItemList:
+      List<QuestionnaireResponse.QuestionnaireResponseItemComponent>,
+  ) {
+    val questionnaireResponseInputItemListIterator = questionnaireResponseInputItemList.iterator()


If these two need to have the same length (which it looks like they do, as we throw an error if they don't on line 137), I would recommend using zip and doing something like this:

require(questionnaireItemList.size == questionnaireResponseInputItemList.size) { "Unequal item amounts: ${questionnaireItemList.size} items and ${questionnaireResponseInputItemList.size} responses" } questionnaireItemList.zip(questionnaireResponseInputItemList) .forEach { (questionnaireItem, questionnaireResponseInputItem) -> ... }

They don't have to be of the same length. The questionnaire response may not have some items . This can happen if the item was disabled / not answered. That's why i removed the zip from the previous implementation.

The error we throw on line 137 is to make sure the questionnaire response does not have more items than the questionnaire. But the questionnaire response can have less number of items. Thats why I am iterating over questionnaireResponseList and using the index to fetch the corresponding questionnaire item. You will find more details regarding this in #836 . Please let me know if it does not make sense.

kevinmost · 2021-12-09T16:56:19Z

...c/main/java/com/google/android/fhir/datacapture/validation/QuestionnaireResponseValidator.kt

+      val questionnaireResponseInputItem = questionnaireResponseInputItemListIterator.next()
+      if (questionnaireItemListIterator.hasNext()) {
+        val questionnaireItem = questionnaireItemListIterator.next()
+        if (!questionnaireItem.linkId.equals(questionnaireResponseInputItem.linkId)) {


Nit: I'd also do this via require, and use ==.

require(questionnaireItem.linkId == questionnaireResponseInputItem.linkId) { "Mismatching..." }

kevinmost · 2021-12-09T16:57:24Z

datacapture/src/test/java/com/google/android/fhir/datacapture/QuestionnaireViewModelTest.kt

+          QuestionnaireResponse.QuestionnaireResponseItemComponent().apply {
+            linkId = "a-link-id"
+            answer =
+              mutableListOf(


Does this need to be mutable?

nope , i made changed it to listOf :)

…ad of .equals

jingtang10 · 2021-12-08T20:07:13Z

...c/main/java/com/google/android/fhir/datacapture/validation/QuestionnaireResponseValidator.kt

+   * items and check if the linkId of the matching pairs of questionnaire item and questionnaire
+   * response item are equal.
+   */
+  fun validateQuestionnaireResponseItems(


Can the function signature be changed to take a questionnaire and a questionnaire response. This funciton itself can probably be a private helper function?

Additionally, how does this function play with the validate function above? I kinda feel at some point we should provide a unified validation API.

Before that though, we should probably change the naming so that it's clear this is a structural validation, and the above function is a validation of answers.

done, I have made this function a private helper and changed the names of the validate functions.

…ction.

kevinmost · 2021-12-15T15:08:00Z

...c/main/java/com/google/android/fhir/datacapture/validation/QuestionnaireResponseValidator.kt

+          questionnaireResponseInputItem.answer.forEachIndexed {
+            index,


Nit: Looks like the index is not needed, could this become forEach instead?

Will take care of it in my next PR. Thanks!

jingtang10

Thanks @joiskash!

I think with this change, we should move a lot of the test cases from view model to the validation package. But that can be done in a follow-up.

joiskash · 2021-12-15T15:28:55Z

Thanks @joiskash!

I think with this change, we should move a lot of the test cases from view model to the validation package. But that can be done in a follow-up.

Yes absolutely! That shouldn't be too hard to do.

joiskash added 2 commits November 17, 2021 15:24

Change validation to check input questionnaire response and the quest…

090d8bd

…ionnaire response created by traversing through the questionnaire

Comment tests to handle case 1. Input questionnaire response has more…

41818c4

… items 2. Input questionnaire Response has less items

joiskash requested review from aditya-07, deepankarb, jingtang10, kevinmost, stevenckngaa and yigit as code owners November 17, 2021 10:03

Handled case where questionnaireResponse that comes via intent has mo…

af4a512

…re items than the questionnaire

joiskash added 4 commits November 17, 2021 18:28

Merge branch 'master' into kj/validation-fix-questionnaireResponse

b5aabf9

Merge branch 'master' into kj/validation-fix-questionnaireResponse

724a643

ran spotless apply

21e37ec

Merge branch 'kj/validation-fix-questionnaireResponse' of https://git…

3283fdc

…hub.com/google/android-fhir into kj/validation-fix-questionnaireResponse

joiskash changed the title ~~Change validateQuestionnaireResponseItems to allow questionnaire repsponse with missing items~~ Change validateQuestionnaireResponseItems to allow questionnaire response with missing items Nov 18, 2021

joiskash self-assigned this Nov 18, 2021

joiskash marked this pull request as draft November 18, 2021 09:07

joiskash added 3 commits November 18, 2021 18:12

Modified validateQuestionnaireResponse to also check type of answer f…

39aaf4f

…or primitives

Added unit tests and change less answer unit test to not throw error

78af019

Fixed bug which caused questionnaire response to always be null

90ca73e

joiskash marked this pull request as ready for review November 18, 2021 12:44

joiskash added 2 commits November 18, 2021 18:14

Merge branch 'master' into kj/validation-fix-questionnaireResponse

0a5d906

Merge branch 'master' into kj/validation-fix-questionnaireResponse

a3da5e6

jingtang10 reviewed Nov 23, 2021

View reviewed changes

joiskash mentioned this pull request Nov 24, 2021

Add safe call in validateQuestionnaireResponseItems to handle custom widget #936

Closed

joiskash added 3 commits November 25, 2021 14:08

Merge branch 'master' into kj/validation-fix-questionnaireResponse

71c46fa

Merge branch 'master' into kj/validation-fix-questionnaireResponse

127b062

Moved validate questionnaire to QuestionnaireResponseValidator.kt and…

8c7cd80

… removed the need to use questionnaire Response for validation.

joiskash added 4 commits December 6, 2021 13:47

Merged changes from PR #937 null check added to validateQuestionnaire…

24d9196

…Response

Use value "type" that has passed the null check instead of using ques…

47b38ed

…tionnaireItem.type

Merge branch 'master' into kj/validation-fix-questionnaireResponse

87ff511

Merge branch 'master' into kj/validation-fix-questionnaireResponse

2280128

jingtang10 assigned kevinmost and jingtang10 Dec 8, 2021

kevinmost reviewed Dec 9, 2021

View reviewed changes

joiskash added 4 commits December 10, 2021 14:00

Using flatMapIndexed instead of asIndexed and flatMap. Using == inste…

6d13df6

…ad of .equals

Using require and == instead of if throws and .equals

0c371a3

Changed mutable list to list

dbd9c93

Merge branch 'master' into kj/validation-fix-questionnaireResponse

133a797

joiskash requested a review from kevinmost December 10, 2021 08:35

jingtang10 requested changes Dec 10, 2021

View reviewed changes

jingtang10 mentioned this pull request Dec 10, 2021

Investigate HAPI instance validation #962

Open

joiskash added 2 commits December 14, 2021 18:54

Merge branch 'master' into kj/validation-fix-questionnaireResponse

619c5ec

Name change to validate function and extraction of private helper fun…

4a860fb

…ction.

joiskash requested a review from jingtang10 December 14, 2021 13:39

Tarun-Bhardwaj linked an issue Dec 14, 2021 that may be closed by this pull request

Crash while using the questionnaire response without disabled items generated by the SDK to resume form filling. #836

Closed

kevinmost approved these changes Dec 15, 2021

View reviewed changes

jingtang10 approved these changes Dec 15, 2021

View reviewed changes

Merge branch 'master' into kj/validation-fix-questionnaireResponse

2a6bf28

jingtang10 enabled auto-merge (squash) December 15, 2021 15:22

jingtang10 merged commit 43ccab2 into master Dec 15, 2021

jingtang10 deleted the kj/validation-fix-questionnaireResponse branch December 15, 2021 17:42

shoaibmushtaq25 mentioned this pull request Jan 20, 2022

Hidden questionnaire items are not handled in the QuestionnaireResponseValidator #1051

Closed

ekigamba mentioned this pull request Jan 27, 2022

Quest - Update QR display and edit functionality with performant option opensrp/fhircore#991

Closed

f-odhiambo mentioned this pull request Feb 1, 2022

Performance profiling - QuestionnaireResponse Extraction optimization #1100

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change validateQuestionnaireResponseItems to allow questionnaire response with missing items #926

Change validateQuestionnaireResponseItems to allow questionnaire response with missing items #926

joiskash commented Nov 17, 2021 •

edited

Loading

joiskash commented Nov 17, 2021

codecov bot commented Nov 18, 2021 •

edited

Loading

jingtang10 left a comment

joiskash commented Nov 23, 2021

jingtang10 commented Dec 8, 2021

kevinmost Dec 9, 2021

joiskash Dec 10, 2021

kevinmost Dec 9, 2021

joiskash Dec 10, 2021

kevinmost Dec 9, 2021

joiskash Dec 9, 2021 •

edited

Loading

joiskash Dec 10, 2021

kevinmost Dec 9, 2021

joiskash Dec 10, 2021

kevinmost Dec 9, 2021

joiskash Dec 10, 2021

jingtang10 Dec 8, 2021

jingtang10 Dec 8, 2021

joiskash Dec 14, 2021

kevinmost Dec 15, 2021

joiskash Dec 15, 2021

jingtang10 left a comment

joiskash commented Dec 15, 2021

		questionnaireResponseInputItem.answer.forEachIndexed {
		index,

Change validateQuestionnaireResponseItems to allow questionnaire response with missing items #926

Change validateQuestionnaireResponseItems to allow questionnaire response with missing items #926

Conversation

joiskash commented Nov 17, 2021 • edited Loading

joiskash commented Nov 17, 2021

codecov bot commented Nov 18, 2021 • edited Loading

Codecov Report

jingtang10 left a comment

Choose a reason for hiding this comment

joiskash commented Nov 23, 2021

jingtang10 commented Dec 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joiskash Dec 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jingtang10 left a comment

Choose a reason for hiding this comment

joiskash commented Dec 15, 2021

joiskash commented Nov 17, 2021 •

edited

Loading

codecov bot commented Nov 18, 2021 •

edited

Loading

joiskash Dec 9, 2021 •

edited

Loading