Add Launch and Submission Timestamps to QR #2672

hamza-vd · 2024-09-12T09:21:00Z

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?

Screenshots (if applicable)

Checklist

I have read and acknowledged the Code of conduct.
I have read the Contributing page.
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 demo app(s) to verify my change fixes the issue and/or does not break the demo app(s).

jingtang10 · 2024-09-17T09:30:10Z

...e/src/main/java/com/google/android/fhir/datacapture/extensions/MoreQuestionnaireResponses.kt

@@ -154,3 +159,21 @@ private fun unpackRepeatedGroups(
    listOf(questionnaireResponseItem)
  }
 }
+
+/** Adds a launch timestamp as an extension to the Questionnaire Response */


can you clarify in the doc that we only add the launch timestamp if there isn't one at the moment?

or should we make this extension repeated so that each time you edit the questionnaire response it is recorded.

useful for audit purposes?

OR if we really want to only keep one (to be a bit more consistent with the authored fied) we can rename this extension to lastLaunched?

In my opinion, the lastLaunched will make more sense. cc: @qiarie

...c/test/java/com/google/android/fhir/datacapture/extensions/MoreQuestionnaireResponsesTest.kt

jingtang10

thanks for creating this new pr @hamza-vd

a few small comments

jingtang10 · 2024-10-01T09:32:55Z

...e/src/main/java/com/google/android/fhir/datacapture/extensions/MoreQuestionnaireResponses.kt

+      .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)] =


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.

@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.

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

jingtang10 · 2024-10-01T09:35:42Z

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

@@ -183,8 +184,23 @@ class QuestionnaireViewModelParameterizedTest(
    val printer: IParser = FhirContext.forR4().newJsonParser()

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


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.

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

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.

@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:

Update all the failing tests to use the same timestamp values for both launch and submission.

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.

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.

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

jingtang10 · 2024-10-01T09:36:56Z

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

@@ -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) {


jingtang10 · 2024-10-08T09:48:09Z

...e/src/main/java/com/google/android/fhir/datacapture/extensions/MoreQuestionnaireResponses.kt

+      .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)] =


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

jingtang10 · 2024-10-08T09:49:25Z

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

@@ -183,8 +184,23 @@ class QuestionnaireViewModelParameterizedTest(
    val printer: IParser = FhirContext.forR4().newJsonParser()

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


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.

jingtang10 · 2024-10-15T10:02:03Z

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

@@ -183,8 +184,23 @@ class QuestionnaireViewModelParameterizedTest(
    val printer: IParser = FhirContext.forR4().newJsonParser()

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


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.

jingtang10 · 2024-10-15T10:03:09Z

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

@@ -183,8 +184,23 @@ class QuestionnaireViewModelParameterizedTest(
    val printer: IParser = FhirContext.forR4().newJsonParser()

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


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

Implementation for launch and submission timestamps addition

cd27970

hamza-vd requested a review from a team as a code owner September 12, 2024 09:21

hamza-vd requested a review from ktarasenko September 12, 2024 09:21

hamza-vd mentioned this pull request Sep 12, 2024

Add Launch and Submission Timestamps to QR #2400

Closed

7 tasks

Merge branch 'master' into launch-and-submission-timestamps

dc8c138

jingtang10 reviewed Sep 17, 2024

View reviewed changes

...c/test/java/com/google/android/fhir/datacapture/extensions/MoreQuestionnaireResponsesTest.kt Outdated Show resolved Hide resolved

jingtang10 reviewed Sep 17, 2024

View reviewed changes

...c/test/java/com/google/android/fhir/datacapture/extensions/MoreQuestionnaireResponsesTest.kt Outdated Show resolved Hide resolved

jingtang10 reviewed Sep 17, 2024

View reviewed changes

...c/test/java/com/google/android/fhir/datacapture/extensions/MoreQuestionnaireResponsesTest.kt Outdated Show resolved Hide resolved

ktarasenko removed their request for review September 17, 2024 14:04

hamza-vd and others added 2 commits September 25, 2024 12:27

Merge branch 'master' into launch-and-submission-timestamps

71d6c3d

Update launch time stamp value to latest

5b0983e

jingtang10 requested changes Oct 1, 2024

View reviewed changes

Merge branch 'master' into launch-and-submission-timestamps

cfdff4c

jingtang10 requested changes Oct 8, 2024

View reviewed changes

hamza-vd and others added 5 commits October 8, 2024 15:52

Merge branch 'master' into launch-and-submission-timestamps

3156fdd

Merge branch 'master' into launch-and-submission-timestamps

86bb30d

Merge branch 'master' into launch-and-submission-timestamps

fea4e20

Merge branch 'master' into launch-and-submission-timestamps

9266784

PR feedback changes

98cb866

hamza-vd requested a review from jingtang10 October 14, 2024 07:18

jingtang10 requested changes Oct 15, 2024

View reviewed changes

Merge branch 'master' into launch-and-submission-timestamps

dfec10d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Launch and Submission Timestamps to QR #2672

Add Launch and Submission Timestamps to QR #2672

hamza-vd commented Sep 12, 2024 •

edited

Loading

jingtang10 Sep 17, 2024

jingtang10 Sep 17, 2024

jingtang10 Sep 17, 2024

hamza-vd Sep 25, 2024

jingtang10 left a comment

jingtang10 Oct 1, 2024

hamza-vd Oct 8, 2024

jingtang10 Oct 8, 2024

hamza-vd Oct 14, 2024

jingtang10 Oct 1, 2024

hamza-vd Oct 8, 2024 •

edited

Loading

jingtang10 Oct 8, 2024

hamza-vd Oct 14, 2024 •

edited

Loading

jingtang10 Oct 15, 2024

jingtang10 Oct 15, 2024

jingtang10 Oct 1, 2024

jingtang10 Oct 8, 2024

jingtang10 Oct 8, 2024

jingtang10 Oct 15, 2024

jingtang10 Oct 15, 2024

		@@ -183,8 +184,23 @@ class QuestionnaireViewModelParameterizedTest(
		val printer: IParser = FhirContext.forR4().newJsonParser()

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

Add Launch and Submission Timestamps to QR #2672

Are you sure you want to change the base?

Add Launch and Submission Timestamps to QR #2672

Conversation

hamza-vd commented Sep 12, 2024 • 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

jingtang10 left a 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

hamza-vd Oct 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hamza-vd Oct 14, 2024 • 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

hamza-vd commented Sep 12, 2024 •

edited

Loading

hamza-vd Oct 8, 2024 •

edited

Loading

hamza-vd Oct 14, 2024 •

edited

Loading