From 2e06cb26f12f33d30167d299a8cc77e5570fda56 Mon Sep 17 00:00:00 2001 From: Jing Tang Date: Tue, 9 Jul 2024 14:01:41 +0100 Subject: [PATCH] Only copy nested items to childless answers (#2600) * Only copy nested items to childless answers * Correct broken test case * Fix bug in questionnaire edit adapter * Suppress lint error --- .../datacapture/QuestionnaireEditAdapter.kt | 25 ++++- .../datacapture/QuestionnaireViewModel.kt | 4 +- .../MoreQuestionnaireItemComponents.kt | 39 +++++-- ...MoreQuestionnaireResponseItemComponents.kt | 22 ++-- .../views/factories/GroupViewHolderFactory.kt | 8 +- .../datacapture/QuestionnaireViewModelTest.kt | 6 +- ...QuestionnaireResponseItemComponentsTest.kt | 102 +++++++++++++++++- 7 files changed, 178 insertions(+), 28 deletions(-) diff --git a/datacapture/src/main/java/com/google/android/fhir/datacapture/QuestionnaireEditAdapter.kt b/datacapture/src/main/java/com/google/android/fhir/datacapture/QuestionnaireEditAdapter.kt index adc80656da..2b2ea328cf 100644 --- a/datacapture/src/main/java/com/google/android/fhir/datacapture/QuestionnaireEditAdapter.kt +++ b/datacapture/src/main/java/com/google/android/fhir/datacapture/QuestionnaireEditAdapter.kt @@ -16,6 +16,7 @@ package com.google.android.fhir.datacapture +import android.annotation.SuppressLint import android.view.View import android.view.ViewGroup import androidx.recyclerview.widget.DiffUtil @@ -328,8 +329,28 @@ internal object DiffCallbacks { QUESTIONS.areContentsTheSame(oldItem, newItem) } is QuestionnaireAdapterItem.RepeatedGroupHeader -> { - newItem is QuestionnaireAdapterItem.RepeatedGroupHeader && - oldItem.responses == newItem.responses + if (newItem is QuestionnaireAdapterItem.RepeatedGroupHeader) { + // The `onDeleteClicked` function is a function closure generated in the questionnaire + // viewmodel with a reference to the parent questionnaire view item. When it is + // invoked, it deletes the current repeated group instance from the parent + // questionnaire view item by removing it from the list of children in the parent + // questionnaire view. + // In other words, although the `onDeleteClicked` function is not a data field, it is + // a function closure with references to data structures. Because + // `RepeatedGroupHeader` does not include any other data fields besides the index, it + // is particularly important to distinguish between different `RepeatedGroupHeader`s + // by the `onDeleteClicked` function. + // If this check is not here, an old RepeatedGroupHeader might be mistakenly + // considered up-to-date and retained in the recycler view even though a newer + // version includes a different `onDeleteClicked` function referencing a parent item + // with a different list of children. As a result clicking the delete function might + // result in deleting from an old list. + @SuppressLint("DiffUtilEquals") + val onDeleteClickedCallbacksEqual = oldItem.onDeleteClicked == newItem.onDeleteClicked + onDeleteClickedCallbacksEqual + } else { + false + } } is QuestionnaireAdapterItem.Navigation -> { newItem is QuestionnaireAdapterItem.Navigation && diff --git a/datacapture/src/main/java/com/google/android/fhir/datacapture/QuestionnaireViewModel.kt b/datacapture/src/main/java/com/google/android/fhir/datacapture/QuestionnaireViewModel.kt index 260d0269df..f806f763d2 100644 --- a/datacapture/src/main/java/com/google/android/fhir/datacapture/QuestionnaireViewModel.kt +++ b/datacapture/src/main/java/com/google/android/fhir/datacapture/QuestionnaireViewModel.kt @@ -28,8 +28,8 @@ import ca.uhn.fhir.parser.IParser import com.google.android.fhir.datacapture.enablement.EnablementEvaluator import com.google.android.fhir.datacapture.expressions.EnabledAnswerOptionsEvaluator import com.google.android.fhir.datacapture.extensions.EntryMode -import com.google.android.fhir.datacapture.extensions.addNestedItemsToAnswer import com.google.android.fhir.datacapture.extensions.allItems +import com.google.android.fhir.datacapture.extensions.copyNestedItemsToChildlessAnswers import com.google.android.fhir.datacapture.extensions.cqfExpression import com.google.android.fhir.datacapture.extensions.createQuestionnaireResponseItem import com.google.android.fhir.datacapture.extensions.entryMode @@ -361,7 +361,7 @@ internal class QuestionnaireViewModel(application: Application, state: SavedStat } } if (questionnaireItem.shouldHaveNestedItemsUnderAnswers) { - questionnaireResponseItem.addNestedItemsToAnswer(questionnaireItem) + questionnaireResponseItem.copyNestedItemsToChildlessAnswers(questionnaireItem) // If nested items are added to the answer, the enablement evaluator needs to be // reinitialized in order for it to rebuild the pre-order map and parent map of diff --git a/datacapture/src/main/java/com/google/android/fhir/datacapture/extensions/MoreQuestionnaireItemComponents.kt b/datacapture/src/main/java/com/google/android/fhir/datacapture/extensions/MoreQuestionnaireItemComponents.kt index fa3f8e6b55..55dd4e5738 100644 --- a/datacapture/src/main/java/com/google/android/fhir/datacapture/extensions/MoreQuestionnaireItemComponents.kt +++ b/datacapture/src/main/java/com/google/android/fhir/datacapture/extensions/MoreQuestionnaireItemComponents.kt @@ -887,29 +887,52 @@ internal val QuestionnaireItemComponent.shouldHaveNestedItemsUnderAnswers: Boole get() = item.isNotEmpty() && (type != Questionnaire.QuestionnaireItemType.GROUP || repeats) /** - * Creates a list of [QuestionnaireResponse.QuestionnaireResponseItemComponent]s from the nested - * items in the [Questionnaire.QuestionnaireItemComponent]. + * Creates a list of [QuestionnaireResponse.QuestionnaireResponseItemComponent]s corresponding to + * the nested items under the questionnaire item. + * + * The list can be added as nested items under answers in a corresponding questionnaire response + * item. This may be because + * 1. the questionnaire item is a question with nested questions, in which case each answer in the + * questionnaire response item needs to have the same nested questions, or + * 2. the questionnaire item is a repeated group, in which case each answer in the questionnaire + * response item represents an instance of the repeated group, and needs to have the same nested + * questions. * * The hierarchy and order of child items will be retained as specified in the standard. See * https://www.hl7.org/fhir/questionnaireresponse.html#notes for more details. */ -fun QuestionnaireItemComponent.getNestedQuestionnaireResponseItems() = +internal fun QuestionnaireItemComponent.createNestedQuestionnaireResponseItems() = item.map { it.createQuestionnaireResponseItem() } /** - * Creates a [QuestionnaireResponse.QuestionnaireResponseItemComponent] from the provided - * [Questionnaire.QuestionnaireItemComponent]. + * Creates a corresponding [QuestionnaireResponse.QuestionnaireResponseItemComponent] for the + * questionnaire item with the following properties: + * - same `linkId` as the questionnaire item, + * - any initial answer(s) specified either in the `initial` element or as `initialSelected` + * `answerOption`(s), + * - any nested questions under the initial answers (there will be no user input yet since this is + * just being created) if this is a question with nested questions, and + * - any nested questions if this is a non-repeated group. + * + * Note that although initial answers to a repeated group may be interpreted as initial instances of + * the repeated group in the in-memory representation of questionnaire response, they are not + * defined as such in the standard. As a result, we are not treating them as such in this function + * to be conformant. * * The hierarchy and order of child items will be retained as specified in the standard. See * https://www.hl7.org/fhir/questionnaireresponse.html#notes for more details. */ -fun QuestionnaireItemComponent.createQuestionnaireResponseItem(): +internal fun QuestionnaireItemComponent.createQuestionnaireResponseItem(): QuestionnaireResponse.QuestionnaireResponseItemComponent { return QuestionnaireResponse.QuestionnaireResponseItemComponent().apply { linkId = this@createQuestionnaireResponseItem.linkId answer = createQuestionnaireResponseItemAnswers() - if (shouldHaveNestedItemsUnderAnswers && answer.isNotEmpty()) { - this.addNestedItemsToAnswer(this@createQuestionnaireResponseItem) + if ( + type != Questionnaire.QuestionnaireItemType.GROUP && + this@createQuestionnaireResponseItem.item.isNotEmpty() && + answer.isNotEmpty() + ) { + this.copyNestedItemsToChildlessAnswers(this@createQuestionnaireResponseItem) } else if ( this@createQuestionnaireResponseItem.type == Questionnaire.QuestionnaireItemType.GROUP && !repeats diff --git a/datacapture/src/main/java/com/google/android/fhir/datacapture/extensions/MoreQuestionnaireResponseItemComponents.kt b/datacapture/src/main/java/com/google/android/fhir/datacapture/extensions/MoreQuestionnaireResponseItemComponents.kt index 977c34dd1f..507abc91e0 100644 --- a/datacapture/src/main/java/com/google/android/fhir/datacapture/extensions/MoreQuestionnaireResponseItemComponents.kt +++ b/datacapture/src/main/java/com/google/android/fhir/datacapture/extensions/MoreQuestionnaireResponseItemComponents.kt @@ -1,5 +1,5 @@ /* - * Copyright 2023 Google LLC + * Copyright 2023-2024 Google LLC * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -38,14 +38,24 @@ private fun QuestionnaireResponse.QuestionnaireResponseItemComponent.appendDesce } /** - * Add nested items under the provided `questionnaireItem` to each answer in the questionnaire - * response item. The hierarchy and order of nested items will be retained as specified in the - * standard. + * Copies nested items under `questionnaireItem` to each answer without children. The hierarchy and + * order of nested items will be retained as specified in the standard. + * + * Existing answers with nested items will not be modified because the nested items may contain + * answers already. + * + * This should be used when + * - a new answer is added to a question with nested questions, or + * - a new answer is added to a repeated group (in which case this indicates a new instance of the + * repeated group will be added to the final questionnaire response). * * See https://www.hl7.org/fhir/questionnaireresponse.html#notes for more details. */ -fun QuestionnaireResponse.QuestionnaireResponseItemComponent.addNestedItemsToAnswer( +internal fun QuestionnaireResponse.QuestionnaireResponseItemComponent + .copyNestedItemsToChildlessAnswers( questionnaireItem: Questionnaire.QuestionnaireItemComponent, ) { - answer.forEach { it.item = questionnaireItem.getNestedQuestionnaireResponseItems() } + answer + .filter { it.item.isEmpty() } + .forEach { it.item = questionnaireItem.createNestedQuestionnaireResponseItems() } } diff --git a/datacapture/src/main/java/com/google/android/fhir/datacapture/views/factories/GroupViewHolderFactory.kt b/datacapture/src/main/java/com/google/android/fhir/datacapture/views/factories/GroupViewHolderFactory.kt index a65b5a2d1c..73243fa0bc 100644 --- a/datacapture/src/main/java/com/google/android/fhir/datacapture/views/factories/GroupViewHolderFactory.kt +++ b/datacapture/src/main/java/com/google/android/fhir/datacapture/views/factories/GroupViewHolderFactory.kt @@ -22,7 +22,6 @@ import android.widget.TextView import androidx.appcompat.app.AppCompatActivity import androidx.lifecycle.lifecycleScope import com.google.android.fhir.datacapture.R -import com.google.android.fhir.datacapture.extensions.getNestedQuestionnaireResponseItems import com.google.android.fhir.datacapture.extensions.tryUnwrapContext import com.google.android.fhir.datacapture.validation.Invalid import com.google.android.fhir.datacapture.validation.NotValidated @@ -62,11 +61,8 @@ internal object GroupViewHolderFactory : addItemButton.setOnClickListener { context.lifecycleScope.launch { questionnaireViewItem.addAnswer( - QuestionnaireResponse.QuestionnaireResponseItemAnswerComponent().apply { - // TODO(jingtang10): This can be removed since we already do this in the - // answerChangedCallback in the QuestionnaireViewModel. - item = questionnaireViewItem.questionnaireItem.getNestedQuestionnaireResponseItems() - }, + // Nested items will be added in answerChangedCallback in the QuestionnaireViewModel + QuestionnaireResponse.QuestionnaireResponseItemAnswerComponent(), ) } } diff --git a/datacapture/src/test/java/com/google/android/fhir/datacapture/QuestionnaireViewModelTest.kt b/datacapture/src/test/java/com/google/android/fhir/datacapture/QuestionnaireViewModelTest.kt index 662773fb31..f4de30897b 100644 --- a/datacapture/src/test/java/com/google/android/fhir/datacapture/QuestionnaireViewModelTest.kt +++ b/datacapture/src/test/java/com/google/android/fhir/datacapture/QuestionnaireViewModelTest.kt @@ -52,8 +52,8 @@ import com.google.android.fhir.datacapture.extensions.EXTENSION_SDC_QUESTIONNAIR import com.google.android.fhir.datacapture.extensions.EXTENSION_VARIABLE_URL import com.google.android.fhir.datacapture.extensions.EntryMode import com.google.android.fhir.datacapture.extensions.asStringValue +import com.google.android.fhir.datacapture.extensions.createNestedQuestionnaireResponseItems import com.google.android.fhir.datacapture.extensions.entryMode -import com.google.android.fhir.datacapture.extensions.getNestedQuestionnaireResponseItems import com.google.android.fhir.datacapture.extensions.logicalId import com.google.android.fhir.datacapture.extensions.maxValue import com.google.android.fhir.datacapture.testing.DataCaptureTestApplication @@ -4332,7 +4332,7 @@ class QuestionnaireViewModelTest { getQuestionnaireAdapterItemListA() .asQuestion() .questionnaireItem - .getNestedQuestionnaireResponseItems() + .createNestedQuestionnaireResponseItems() }, ) getQuestionnaireAdapterItemListB() @@ -4343,7 +4343,7 @@ class QuestionnaireViewModelTest { getQuestionnaireAdapterItemListB() .asQuestion() .questionnaireItem - .getNestedQuestionnaireResponseItems() + .createNestedQuestionnaireResponseItems() }, ) } diff --git a/datacapture/src/test/java/com/google/android/fhir/datacapture/extensions/MoreQuestionnaireResponseItemComponentsTest.kt b/datacapture/src/test/java/com/google/android/fhir/datacapture/extensions/MoreQuestionnaireResponseItemComponentsTest.kt index b4a9ed4500..6cb8eee3e6 100644 --- a/datacapture/src/test/java/com/google/android/fhir/datacapture/extensions/MoreQuestionnaireResponseItemComponentsTest.kt +++ b/datacapture/src/test/java/com/google/android/fhir/datacapture/extensions/MoreQuestionnaireResponseItemComponentsTest.kt @@ -1,5 +1,5 @@ /* - * Copyright 2022-2023 Google LLC + * Copyright 2022-2024 Google LLC * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -17,6 +17,10 @@ package com.google.android.fhir.datacapture.extensions import com.google.common.truth.Truth.assertThat +import org.hl7.fhir.r4.model.DecimalType +import org.hl7.fhir.r4.model.IntegerType +import org.hl7.fhir.r4.model.Questionnaire +import org.hl7.fhir.r4.model.Questionnaire.QuestionnaireItemComponent import org.hl7.fhir.r4.model.QuestionnaireResponse import org.hl7.fhir.r4.model.QuestionnaireResponse.QuestionnaireResponseItemComponent import org.junit.Test @@ -67,4 +71,100 @@ class MoreQuestionnaireResponseItemComponentsTest { questionnaireResponseItem22, ) } + + @Test + fun `should copy nested item to childless answer`() { + val questionnaireItem = + QuestionnaireItemComponent().apply { + linkId = "question" + type = Questionnaire.QuestionnaireItemType.GROUP + repeats = true + addItem( + QuestionnaireItemComponent().apply { + linkId = "nested-question-1" + type = Questionnaire.QuestionnaireItemType.INTEGER + }, + ) + addItem( + QuestionnaireItemComponent().apply { + linkId = "nested-question-2" + type = Questionnaire.QuestionnaireItemType.DECIMAL + }, + ) + } + val questionnaireResponseItem = + QuestionnaireResponseItemComponent().apply { + linkId = "question" + addAnswer(QuestionnaireResponse.QuestionnaireResponseItemAnswerComponent()) + } + + questionnaireResponseItem.copyNestedItemsToChildlessAnswers(questionnaireItem) + + val nestedItems = questionnaireResponseItem.answer.single().item + assertThat(nestedItems).hasSize(2) + assertThat(nestedItems.map { it.linkId }) + .containsExactly("nested-question-1", "nested-question-2") + assertThat(nestedItems.first().answer).isEmpty() + assertThat(nestedItems.last().answer).isEmpty() + } + + @Test + fun `should not copy nested item to existing answer with children`() { + val questionnaireItem = + QuestionnaireItemComponent().apply { + linkId = "question" + type = Questionnaire.QuestionnaireItemType.GROUP + repeats = true + addItem( + QuestionnaireItemComponent().apply { + linkId = "nested-question-1" + type = Questionnaire.QuestionnaireItemType.INTEGER + }, + ) + addItem( + QuestionnaireItemComponent().apply { + linkId = "nested-question-2" + type = Questionnaire.QuestionnaireItemType.DECIMAL + }, + ) + } + val questionnaireResponseItem = + QuestionnaireResponseItemComponent().apply { + linkId = "question" + addAnswer( + QuestionnaireResponse.QuestionnaireResponseItemAnswerComponent().apply { + addItem( + QuestionnaireResponseItemComponent().apply { + linkId = "nested-question-1" + addAnswer( + QuestionnaireResponse.QuestionnaireResponseItemAnswerComponent().apply { + value = IntegerType(1) + }, + ) + }, + ) + addItem( + QuestionnaireResponseItemComponent().apply { + linkId = "nested-question-2" + addAnswer( + QuestionnaireResponse.QuestionnaireResponseItemAnswerComponent().apply { + value = DecimalType(1.0) + }, + ) + }, + ) + }, + ) + } + + questionnaireResponseItem.copyNestedItemsToChildlessAnswers(questionnaireItem) + + val nestedItems = questionnaireResponseItem.answer.single().item + assertThat(nestedItems).hasSize(2) + assertThat(nestedItems.map { it.linkId }) + .containsExactly("nested-question-1", "nested-question-2") + assertThat(nestedItems.first().answer.single().valueIntegerType.value).isEqualTo(1) + assertThat(nestedItems.last().answer.single().valueDecimalType.value.toString()) + .isEqualTo("1.0") + } }