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

Jump to bottom

Add Launch and Submission Timestamps to QR #2672

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Add Launch and Submission Timestamps to QR #2672

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
cd27970
Implementation for launch and submission timestamps addition
hamza-vd Sep 12, 2024
dc8c138
Merge branch 'master' into launch-and-submission-timestamps
qiarie Sep 12, 2024
71d6c3d
Merge branch 'master' into launch-and-submission-timestamps
hamza-vd Sep 25, 2024
5b0983e
Update launch time stamp value to latest
hamza-vd Sep 25, 2024
cfdff4c
Merge branch 'master' into launch-and-submission-timestamps
qiarie Oct 3, 2024
3156fdd
Merge branch 'master' into launch-and-submission-timestamps
hamza-vd Oct 8, 2024
86bb30d
Merge branch 'master' into launch-and-submission-timestamps
hamza-vd Oct 9, 2024
fea4e20
Merge branch 'master' into launch-and-submission-timestamps
qiarie Oct 11, 2024
9266784
Merge branch 'master' into launch-and-submission-timestamps
qiarie Oct 11, 2024
98cb866
PR feedback changes
hamza-vd Oct 14, 2024
dfec10d
Merge branch 'master' into launch-and-submission-timestamps
qiarie Oct 17, 2024
809a27b
Merge branch 'master' into launch-and-submission-timestamps
hamza-vd Oct 18, 2024
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
7 changes: 7 additions & 0 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 @@ -42,6 +42,7 @@ import com.google.android.fhir.datacapture.extensions.isHelpCode
import com.google.android.fhir.datacapture.extensions.isHidden
import com.google.android.fhir.datacapture.extensions.isPaginated
import com.google.android.fhir.datacapture.extensions.isRepeatedGroup
import com.google.android.fhir.datacapture.extensions.launchTimestamp
import com.google.android.fhir.datacapture.extensions.localizedTextSpanned
import com.google.android.fhir.datacapture.extensions.maxValue
import com.google.android.fhir.datacapture.extensions.maxValueCqfCalculatedValueExpression
Expand All @@ -64,6 +65,7 @@ import com.google.android.fhir.datacapture.validation.Valid
import com.google.android.fhir.datacapture.validation.ValidationResult
import com.google.android.fhir.datacapture.views.QuestionTextConfiguration
import com.google.android.fhir.datacapture.views.QuestionnaireViewItem
import java.util.Date
import kotlinx.coroutines.flow.MutableStateFlow
import kotlinx.coroutines.flow.SharingStarted
import kotlinx.coroutines.flow.StateFlow
Expand All @@ -74,6 +76,7 @@ import kotlinx.coroutines.flow.stateIn
import kotlinx.coroutines.flow.update
import kotlinx.coroutines.flow.withIndex
import kotlinx.coroutines.launch
import org.hl7.fhir.r4.model.DateTimeType
import org.hl7.fhir.r4.model.Questionnaire
import org.hl7.fhir.r4.model.Questionnaire.QuestionnaireItemComponent
import org.hl7.fhir.r4.model.QuestionnaireResponse
Expand Down Expand Up @@ -160,6 +163,8 @@ internal class QuestionnaireViewModel(application: Application, state: SavedStat
.forEach { questionnaireResponse.addItem(it.createQuestionnaireResponseItem()) }
}
}
// Add extension for questionnaire launch time stamp
questionnaireResponse.launchTimestamp = DateTimeType(Date())
questionnaireResponse.packRepeatedGroups(questionnaire)
}

Expand Down Expand Up @@ -475,6 +480,8 @@ internal class QuestionnaireViewModel(application: Application, state: SavedStat
)
.map { it.copy() }
unpackRepeatedGroups([email protected])
// Use authored as a submission time stamp
authored = Date()
}
}

Expand Down
28 changes: 28 additions & 0 deletions ...rc/main/java/com/google/android/fhir/datacapture/extensions/MoreQuestionnaireResponses.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,14 @@

package com.google.android.fhir.datacapture.extensions

import org.hl7.fhir.r4.model.DateTimeType
import org.hl7.fhir.r4.model.Extension
import org.hl7.fhir.r4.model.Questionnaire
import org.hl7.fhir.r4.model.QuestionnaireResponse

internal const val EXTENSION_LAST_LAUNCHED_TIMESTAMP: String =
"http://github.com/google-android/questionnaire-lastLaunched-timestamp"

/** Pre-order list of all questionnaire response items in the questionnaire. */
val QuestionnaireResponse.allItems: List<QuestionnaireResponse.QuestionnaireResponseItemComponent>
get() = item.flatMap { it.descendant }
Expand Down Expand Up @@ -154,3 +159,26 @@ private fun unpackRepeatedGroups(
listOf(questionnaireResponseItem)
}
}

/**
* Adds a launch timestamp extension to the Questionnaire Response. If the extension @see
* EXTENSION_LAUNCH_TIMESTAMP already exists, it updates its value; otherwise, it adds a new one.
*/
internal var QuestionnaireResponse.launchTimestamp: DateTimeType?
get() {
val extension = this.extension.firstOrNull { it.url == EXTENSION_LAST_LAUNCHED_TIMESTAMP }
return extension?.value as? DateTimeType
}
set(value) {
this.extension
.find { it.url == EXTENSION_LAST_LAUNCHED_TIMESTAMP }
?.let {
// Replace the existing extension with a new one having the updated value
this.extension[this.extension.indexOf(it)] =
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we just overwrite the value here?

the whole operation of this this.extension[this.extension.indexOf(it)] seems to negate the benefit of using scope functions... looks a bit more clumsy in my opinion.

Copy link
Author

Choose a reason for hiding this comment

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

@jingtang10 this is because we need to keep the previous extensions intact and only update the url for a specific extension. The url in extension is a immutable property so I had to assign a new one.

Copy link
Collaborator

Choose a reason for hiding this comment

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

i mean can you just overwrite th value without changeing the url? the url is already the same (as it has been checked in line 174).

so do somethign like this.extension.setValue

Copy link
Author

Choose a reason for hiding this comment

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

Updated

Extension(EXTENSION_LAST_LAUNCHED_TIMESTAMP, value)
}
?: run {
// Add a new extension if none exists
this.extension.add(Extension(EXTENSION_LAST_LAUNCHED_TIMESTAMP, value))
}
}
20 changes: 18 additions & 2 deletions .../test/java/com/google/android/fhir/datacapture/QuestionnaireViewModelParameterizedTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import com.google.android.fhir.datacapture.QuestionnaireFragment.Companion.EXTRA
import com.google.android.fhir.datacapture.QuestionnaireFragment.Companion.EXTRA_QUESTIONNAIRE_RESPONSE_JSON_STRING
import com.google.android.fhir.datacapture.QuestionnaireFragment.Companion.EXTRA_QUESTIONNAIRE_RESPONSE_JSON_URI
import com.google.android.fhir.datacapture.QuestionnaireFragment.Companion.EXTRA_SHOW_REVIEW_PAGE_FIRST
import com.google.android.fhir.datacapture.extensions.EXTENSION_LAST_LAUNCHED_TIMESTAMP
import com.google.android.fhir.datacapture.testing.DataCaptureTestApplication
import com.google.common.truth.Truth.assertThat
import java.io.File
Expand Down Expand Up @@ -183,8 +184,23 @@ class QuestionnaireViewModelParameterizedTest(
val printer: IParser = FhirContext.forR4().newJsonParser()

fun <T : IBaseResource> assertResourceEquals(actual: T, expected: T) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

as @ndegwamartin pointed out, might as well split this assert function into two to make it clearer and use the right one in the relevant tests...

i think at the moment this code might hide errors and make debugging difficult in the future.

Copy link
Author

@hamza-vd hamza-vd Oct 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@ndegwamartin can you please elaborate further on how can we split the assertion here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

create a new function to assert two questionnaire responses in which you fiddle with the timestamps.

this assertion is used by a lot of tests. seems at least inefficient to do this operation for all resource types.

Copy link
Author

@hamza-vd hamza-vd Oct 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@jingtang10 @ndegwamartin cc: @qiarie

The updates I made to this method were not to test my own code, but to ensure the failing tests would pass. You can find the tests for my implementation here.

After adding timestamps to the QuestionnaireResponses, many tests that asserted response equality started failing due to mismatching timestamps. Although the difference was only in milliseconds, it still caused the checks to fail. This left me with two options:

  1. Update all the failing tests to use the same timestamp values for both launch and submission.
  2. Modify the assertion method to programmatically adjust the expected and actual response timestamps.

I chose the second approach as it was more time-efficient, and we already have separate tests for timestamp functionality. I hope this makes sense.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The problem is that the function is now doing things that is quite specific to questionnaire responses and someone in the future is going to call the assertResouceEquals function and assume that it's just testing two resoures are equal but it's not... From a code health's perspective this is an anti pattern... we should make sure that the function does what the name suggests that it does.

Please create a function called assertQuestionnaireResponseEqualsIgnoringLaunchTimestamp which takes two QuestionnaireResponses. In the test cases that you are seeing that fail - they should use this new funciton instead of the old function assertResourceEquals.

I think this change shouldn't be too big - it should be easy to find and replace usage of this function with questionnaire responses and use the new funciton.

Copy link
Collaborator

Choose a reason for hiding this comment

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

also make sure you don't mutate the resources - make a copy and then remove the timestamp.

assertThat(printer.encodeResourceToString(actual))
.isEqualTo(printer.encodeResourceToString(expected))
if (actual is QuestionnaireResponse && expected is QuestionnaireResponse) {
val actualResponse = (actual as QuestionnaireResponse)
val expectedResponse =
(expected as QuestionnaireResponse).apply {
extension.add(
actualResponse.extension.firstOrNull { extension ->
extension.url == EXTENSION_LAST_LAUNCHED_TIMESTAMP
},
)
authored = actualResponse.authored
}
assertThat(printer.encodeResourceToString(actualResponse))
.isEqualTo(printer.encodeResourceToString(expectedResponse))
} else {
assertThat(printer.encodeResourceToString(actual))
.isEqualTo(printer.encodeResourceToString(expected))
}
}

@JvmStatic
Expand Down
22 changes: 19 additions & 3 deletions datacapture/src/test/java/com/google/android/fhir/datacapture/QuestionnaireViewModelTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ import com.google.android.fhir.datacapture.extensions.EXTENSION_ENTRY_MODE_URL
import com.google.android.fhir.datacapture.extensions.EXTENSION_HIDDEN_URL
import com.google.android.fhir.datacapture.extensions.EXTENSION_ITEM_CONTROL_SYSTEM
import com.google.android.fhir.datacapture.extensions.EXTENSION_ITEM_CONTROL_URL
import com.google.android.fhir.datacapture.extensions.EXTENSION_LAST_LAUNCHED_TIMESTAMP
import com.google.android.fhir.datacapture.extensions.EXTENSION_SDC_QUESTIONNAIRE_LAUNCH_CONTEXT
import com.google.android.fhir.datacapture.extensions.EXTENSION_VARIABLE_URL
import com.google.android.fhir.datacapture.extensions.EntryMode
Expand Down Expand Up @@ -651,7 +652,7 @@ class QuestionnaireViewModelTest {

val viewModel = createQuestionnaireViewModel(questionnaire, questionnaireResponse)

runTest { assertResourceEquals(questionnaireResponse, viewModel.getQuestionnaireResponse()) }
runTest { assertResourceEquals(viewModel.getQuestionnaireResponse(), questionnaireResponse) }
}

@Test
Expand Down Expand Up @@ -7514,8 +7515,23 @@ class QuestionnaireViewModelTest {
val printer: IParser = FhirContext.forR4().newJsonParser()

fun <T : IBaseResource> assertResourceEquals(actual: T, expected: T) {
assertThat(printer.encodeResourceToString(actual))
.isEqualTo(printer.encodeResourceToString(expected))
if (actual is QuestionnaireResponse && expected is QuestionnaireResponse) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

same here.

val actualResponse = (actual as QuestionnaireResponse)
val expectedResponse =
(expected as QuestionnaireResponse).apply {
extension.add(
actualResponse.extension.firstOrNull { extension ->
extension.url == EXTENSION_LAST_LAUNCHED_TIMESTAMP
},
)
authored = actualResponse.authored
}
assertThat(printer.encodeResourceToString(actualResponse))
.isEqualTo(printer.encodeResourceToString(expectedResponse))
} else {
assertThat(printer.encodeResourceToString(actual))
.isEqualTo(printer.encodeResourceToString(expected))
}
}
}
}
Expand Down
26 changes: 26 additions & 0 deletions ...est/java/com/google/android/fhir/datacapture/extensions/MoreQuestionnaireResponsesTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import ca.uhn.fhir.context.FhirContext
import ca.uhn.fhir.context.FhirVersionEnum
import com.google.common.truth.Truth.assertThat
import org.hl7.fhir.r4.model.BooleanType
import org.hl7.fhir.r4.model.DateTimeType
import org.hl7.fhir.r4.model.Questionnaire
import org.hl7.fhir.r4.model.Questionnaire.QuestionnaireItemComponent
import org.hl7.fhir.r4.model.QuestionnaireResponse
Expand Down Expand Up @@ -695,4 +696,29 @@ class MoreQuestionnaireResponsesTest {
assertThat(iParser.encodeResourceToString(actual))
.isEqualTo(iParser.encodeResourceToString(expected))
}

@Test
fun `should add launchTimestamp`() {
val questionnaireResponse = QuestionnaireResponse()
val dateTimeType = DateTimeType("2024-07-05T00:00:00Z")
questionnaireResponse.launchTimestamp = dateTimeType

assertThat(dateTimeType).isEqualTo(questionnaireResponse.launchTimestamp)
}

@Test
fun `launchTimestamp should be null when not added`() {
assertThat(QuestionnaireResponse().launchTimestamp).isNull()
}

@Test
fun `launchTimestamp should update if already exists`() {
val questionnaireResponse = QuestionnaireResponse()
val oldDateTimeType = DateTimeType("2024-07-01T00:00:00Z")
val newDateTimeType = DateTimeType("2024-07-05T00:00:00Z")
questionnaireResponse.launchTimestamp = oldDateTimeType
questionnaireResponse.launchTimestamp = newDateTimeType

assertThat(newDateTimeType).isEqualTo(questionnaireResponse.launchTimestamp)
}
}
Loading