Skip to content
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.

Sign up for GitHub

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

Change validateQuestionnaireResponseItems to allow questionnaire response with missing items #926

Merged
merged 38 commits into from
Dec 15, 2021
Merged

Change validateQuestionnaireResponseItems to allow questionnaire response with missing items #926

Show file tree
Hide file tree
Changes from 37 commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
090d8bd
Change validation to check input questionnaire response and the quest…
joiskash Nov 17, 2021
41818c4
Comment tests to handle case 1. Input questionnaire response has more…
joiskash Nov 17, 2021
af4a512
Handled case where questionnaireResponse that comes via intent has mo…
joiskash Nov 17, 2021
b5aabf9
Merge branch 'master' into kj/validation-fix-questionnaireResponse
joiskash Nov 17, 2021
724a643
Merge branch 'master' into kj/validation-fix-questionnaireResponse
joiskash Nov 18, 2021
21e37ec
ran spotless apply
joiskash Nov 18, 2021
3283fdc
Merge branch 'kj/validation-fix-questionnaireResponse' of https://git…
joiskash Nov 18, 2021
39aaf4f
Modified validateQuestionnaireResponse to also check type of answer f…
joiskash Nov 18, 2021
78af019
Added unit tests and change less answer unit test to not throw error
joiskash Nov 18, 2021
90ca73e
Fixed bug which caused questionnaire response to always be null
joiskash Nov 18, 2021
0a5d906
Merge branch 'master' into kj/validation-fix-questionnaireResponse
joiskash Nov 18, 2021
a3da5e6
Merge branch 'master' into kj/validation-fix-questionnaireResponse
joiskash Nov 23, 2021
71c46fa
Merge branch 'master' into kj/validation-fix-questionnaireResponse
joiskash Nov 25, 2021
127b062
Merge branch 'master' into kj/validation-fix-questionnaireResponse
joiskash Nov 30, 2021
8c7cd80
Moved validate questionnaire to QuestionnaireResponseValidator.kt and…
joiskash Nov 30, 2021
82857a1
Merge branch 'master' into kj/validation-fix-questionnaireResponse
joiskash Nov 30, 2021
b42d79a
Changed order of parameters to keep it the same as before
joiskash Dec 1, 2021
c38fd17
Reverted to the old way of initializing questionnaire response and ad…
joiskash Dec 1, 2021
d816628
Merge branch 'kj/validation-fix-questionnaireResponse' of https://git…
joiskash Dec 1, 2021
62473cb
Merge branch 'master' into kj/validation-fix-questionnaireResponse
joiskash Dec 1, 2021
c782933
Change unit test for input questionnaire response with missing items
joiskash Dec 1, 2021
482d1f0
Merge branch 'kj/validation-fix-questionnaireResponse' of https://git…
joiskash Dec 1, 2021
dcbd36a
Ran spotlessApply
joiskash Dec 1, 2021
7204dee
Merge branch 'master' into kj/validation-fix-questionnaireResponse
joiskash Dec 2, 2021
8a92d03
Bumped up test coverage
joiskash Dec 3, 2021
786872e
Merge branch 'kj/validation-fix-questionnaireResponse' of https://git…
joiskash Dec 3, 2021
854710d
Merge branch 'master' into kj/validation-fix-questionnaireResponse
joiskash Dec 6, 2021
24d9196
Merged changes from PR #937 null check added to validateQuestionnaire…
joiskash Dec 6, 2021
47b38ed
Use value "type" that has passed the null check instead of using ques…
joiskash Dec 6, 2021
87ff511
Merge branch 'master' into kj/validation-fix-questionnaireResponse
joiskash Dec 6, 2021
2280128
Merge branch 'master' into kj/validation-fix-questionnaireResponse
joiskash Dec 8, 2021
6d13df6
Using flatMapIndexed instead of asIndexed and flatMap. Using == inste…
joiskash Dec 10, 2021
0c371a3
Using require and == instead of if throws and .equals
joiskash Dec 10, 2021
dbd9c93
Changed mutable list to list
joiskash Dec 10, 2021
133a797
Merge branch 'master' into kj/validation-fix-questionnaireResponse
joiskash Dec 10, 2021
619c5ec
Merge branch 'master' into kj/validation-fix-questionnaireResponse
joiskash Dec 14, 2021
4a860fb
Name change to validate function and extraction of private helper fun…
joiskash Dec 14, 2021
2a6bf28
Merge branch 'master' into kj/validation-fix-questionnaireResponse
jingtang10 Dec 15, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 16 additions & 56 deletions datacapture/src/main/java/com/google/android/fhir/datacapture/QuestionnaireViewModel.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import androidx.lifecycle.SavedStateHandle
import androidx.lifecycle.viewModelScope
import ca.uhn.fhir.context.FhirContext
import com.google.android.fhir.datacapture.enablement.EnablementEvaluator
import com.google.android.fhir.datacapture.validation.QuestionnaireResponseValidator.validateQuestionnaireResponseStructure
import com.google.android.fhir.datacapture.views.QuestionnaireItemViewItem
import kotlinx.coroutines.flow.Flow
import kotlinx.coroutines.flow.MutableStateFlow
Expand Down Expand Up @@ -79,7 +80,7 @@ internal class QuestionnaireViewModel(application: Application, state: SavedStat
questionnaireResponse =
FhirContext.forR4().newJsonParser().parseResource(questionnaireJsonResponseString) as
QuestionnaireResponse
validateQuestionnaireResponseItems(questionnaire.item, questionnaireResponse.item)
validateQuestionnaireResponseStructure(questionnaire, questionnaireResponse)
} else {
questionnaireResponse =
QuestionnaireResponse().apply {
Expand Down Expand Up @@ -249,18 +250,25 @@ 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()
.zip(questionnaireResponseItemList.asSequence())
.flatMap { (questionnaireItemAndIndex, questionnaireResponseItem) ->
val (index, questionnaireItem) = questionnaireItemAndIndex

.flatMapIndexed { index, questionnaireItem ->
var questionnaireResponseItem = questionnaireItem.createQuestionnaireResponseItem()

// 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 == questionnaireResponseItem.linkId
) {
questionnaireResponseItem = questionnaireResponseItemList[responseIndex]
responseIndex += 1
}
// if the questionnaire is paginated and we're currently working through the paginated
// groups, make sure that only the current page gets set
if (pagination != null && pagination.currentPageIndex != index) {
return@flatMap emptyList()
return@flatMapIndexed emptyList()
}

val enabled =
Expand All @@ -269,7 +277,7 @@ internal class QuestionnaireViewModel(application: Application, state: SavedStat
}

if (!enabled || questionnaireItem.isHidden) {
return@flatMap emptyList()
return@flatMapIndexed emptyList()
}

listOf(
Expand Down Expand Up @@ -321,54 +329,6 @@ internal class QuestionnaireViewModel(application: Application, state: SavedStat
.toList()
}

/**
* Traverse (DFS) through the list of questionnaire items and the list of questionnaire response
* items and check if the linkId of the matching pairs of questionnaire item and questionnaire
* response item are equal. The traverse is carried out in the two lists in tandem. The two lists
* should be structurally identical.
*/
private fun validateQuestionnaireResponseItems(
questionnaireItemList: List<Questionnaire.QuestionnaireItemComponent>,
questionnaireResponseItemList: List<QuestionnaireResponse.QuestionnaireResponseItemComponent>
) {
val questionnaireItemListIterator = questionnaireItemList.iterator()
val questionnaireResponseItemListIterator = questionnaireResponseItemList.iterator()
while (questionnaireItemListIterator.hasNext() &&
questionnaireResponseItemListIterator.hasNext()) {
// TODO: Validate type and item nesting within answers for repeated answers
// https://github.com/google/android-fhir/issues/286
val questionnaireItem = questionnaireItemListIterator.next()
val questionnaireResponseItem = questionnaireResponseItemListIterator.next()
if (!questionnaireItem.linkId.equals(questionnaireResponseItem.linkId))
throw IllegalArgumentException(
"Mismatching linkIds for questionnaire item ${questionnaireItem.linkId} and " +
"questionnaire response item ${questionnaireResponseItem.linkId}"
)
val type = checkNotNull(questionnaireItem.type) { "Questionnaire item must have type" }
if (type == Questionnaire.QuestionnaireItemType.GROUP) {
validateQuestionnaireResponseItems(questionnaireItem.item, questionnaireResponseItem.item)
} else {
if (questionnaireResponseItem.answer.isNotEmpty())
validateQuestionnaireResponseItems(
questionnaireItem.item,
questionnaireResponseItem.answer.first().item
)
}
}
if (questionnaireItemListIterator.hasNext() xor questionnaireResponseItemListIterator.hasNext()
) {
if (questionnaireItemListIterator.hasNext()) {
throw IllegalArgumentException(
"No matching questionnaire response item for questionnaire item ${questionnaireItemListIterator.next().linkId}"
)
} else {
throw IllegalArgumentException(
"No matching questionnaire item for questionnaire response item ${questionnaireResponseItemListIterator.next().linkId}"
)
}
}
}

/**
* Checks if this questionnaire uses pagination via the "page" extension.
*
Expand Down
100 changes: 97 additions & 3 deletions ...ain/java/com/google/android/fhir/datacapture/validation/QuestionnaireResponseValidator.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ object QuestionnaireResponseValidator {
* Validates [questionnaireResponseItemList] using the constraints defined in the
* [questionnaireItemList].
*/
fun validate(
fun validateQuestionnaireResponseAnswers(
questionnaireItemList: List<Questionnaire.QuestionnaireItemComponent>,
questionnaireResponseItemList: List<QuestionnaireResponse.QuestionnaireResponseItemComponent>,
context: Context
Expand All @@ -53,10 +53,104 @@ object QuestionnaireResponseValidator {
)
if (questionnaireItem.hasNestedItemsWithinAnswers) {
// TODO(https://github.com/google/android-fhir/issues/487): Validates all answers.
validate(questionnaireItem.item, questionnaireResponseItem.answer[0].item, context)
validateQuestionnaireResponseAnswers(
questionnaireItem.item,
questionnaireResponseItem.answer[0].item,
context
)
}
validate(questionnaireItem.item, questionnaireResponseItem.item, context)
validateQuestionnaireResponseAnswers(
questionnaireItem.item,
questionnaireResponseItem.item,
context
)
}
return linkIdToValidationResultMap
}

/**
* Traverse (DFS) through the [Questionnaire.item] list and the [QuestionnaireResponse.item] list
* to check if the linkId of the matching pairs of questionnaire item and questionnaire response
* item are equal.
*/
fun validateQuestionnaireResponseStructure(
questionnaire: Questionnaire,
questionnaireResponse: QuestionnaireResponse
) {
validateQuestionnaireResponseItemsStructurally(questionnaire.item, questionnaireResponse.item)
}

private fun validateQuestionnaireResponseItemsStructurally(
questionnaireItemList: List<Questionnaire.QuestionnaireItemComponent>,
questionnaireResponseInputItemList:
List<QuestionnaireResponse.QuestionnaireResponseItemComponent>,
) {
val questionnaireResponseInputItemListIterator = questionnaireResponseInputItemList.iterator()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) -> ... }

Copy link
Collaborator Author

@joiskash joiskash Dec 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

val questionnaireItemListIterator = questionnaireItemList.iterator()

while (questionnaireResponseInputItemListIterator.hasNext()) {
// TODO: Validate type and item nesting within answers for repeated answers
// https://github.com/google/android-fhir/issues/286
val questionnaireResponseInputItem = questionnaireResponseInputItemListIterator.next()
if (questionnaireItemListIterator.hasNext()) {
val questionnaireItem = questionnaireItemListIterator.next()
require(questionnaireItem.linkId == questionnaireResponseInputItem.linkId) {
"Mismatching linkIds for questionnaire item ${questionnaireItem.linkId} and " +
"questionnaire response item ${questionnaireResponseInputItem.linkId}"
}
val type = checkNotNull(questionnaireItem.type) { "Questionnaire item must have type" }
if (questionnaireResponseInputItem.hasAnswer() &&
type != Questionnaire.QuestionnaireItemType.GROUP
) {
if (!questionnaireItem.repeats && questionnaireResponseInputItem.answer.size > 1) {
throw IllegalArgumentException(
"Multiple answers in ${questionnaireResponseInputItem.linkId} and repeats false in " +
"questionnaire item ${questionnaireItem.linkId}"
)
}
questionnaireResponseInputItem.answer.forEachIndexed {
index,
Comment on lines +111 to +112
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

questionnaireResponseItemAnswerComponent ->
if (questionnaireResponseItemAnswerComponent.hasValue()) {
when (type) {
Questionnaire.QuestionnaireItemType.BOOLEAN,
Questionnaire.QuestionnaireItemType.DECIMAL,
Questionnaire.QuestionnaireItemType.INTEGER,
Questionnaire.QuestionnaireItemType.DATE,
Questionnaire.QuestionnaireItemType.DATETIME,
Questionnaire.QuestionnaireItemType.TIME,
Questionnaire.QuestionnaireItemType.STRING,
Questionnaire.QuestionnaireItemType.URL ->
if (!questionnaireResponseItemAnswerComponent
.value
.fhirType()
.equals(type.toCode())
) {
throw IllegalArgumentException(
"Type mismatch for linkIds for questionnaire item ${questionnaireItem.linkId} and " +
"questionnaire response item ${questionnaireResponseInputItem.linkId}"
)
}
else -> Unit // Check type for primitives only
}
}
validateQuestionnaireResponseItemsStructurally(
questionnaireItem.item,
questionnaireResponseItemAnswerComponent.item
)
}
} else if (questionnaireResponseInputItem.hasItem()) {
validateQuestionnaireResponseItemsStructurally(
questionnaireItem.item,
questionnaireResponseInputItem.item
)
}
} else {
// Input response has more items
throw IllegalArgumentException(
"No matching questionnaire item for questionnaire response item ${questionnaireResponseInputItem.linkId}"
)
}
}
}
}
Loading