Change areItemsTheSame to check if internal objects are the same with === #1185

joiskash · 2022-02-21T12:55:05Z

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

Fixes #765

Description
If you have a questionnaire with multiple levels of nesting, questionnaire response items are nested within answers. When the parent answer is changed, we clear the questionnaireResponseItem.answer list and add the new answer to it. The nested questionnaireResponseItems also get cleared and new instances of them are created in the questionnaireResponseItemChangedCallback. Since questionnaireItemViewItem.questionnaireResponseItem was a value , the ViewHolderFactory would always hold the reference to the old nested questionnaireResponseItem. On making it a variable, the viewHolderFactory now gets the reference of the correct questionnaireResponseitem while setting the answer. This allows enable when to work properly and thus displays the third nested answer too.

Alternative(s) considered
No

Type
Choose one: Bug fix

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 reference app(s) to verify my change fixes the issue and/or does not break the reference app(s)

…allback

…nswerChanged calls this internally

… which may contain nested questionnaireReponseItems

codecov · 2022-02-21T13:51:18Z

Codecov Report

Merging #1185 (23782b0) into master (e6bb4c4) will increase coverage by 0.00%.
The diff coverage is 62.50%.

@@            Coverage Diff            @@
##             master    #1185   +/-   ##
=========================================
  Coverage     84.57%   84.57%           
- Complexity      627      632    +5     
=========================================
  Files           135      135           
  Lines         10404    10407    +3     
  Branches        786      787    +1     
=========================================
+ Hits           8799     8802    +3     
+ Misses         1210     1209    -1     
- Partials        395      396    +1

Impacted Files	Coverage Δ
...QuestionnaireItemBarCodeReaderViewHolderFactory.kt	`36.73% <0.00%> (ø)`
...iews/QuestionnaireItemDropDownViewHolderFactory.kt	`50.00% <ø> (+1.35%)`	⬆️
...hir/datacapture/views/QuestionnaireItemViewItem.kt	`88.23% <60.00%> (-4.87%)`	⬇️
...droid/fhir/datacapture/QuestionnaireItemAdapter.kt	`61.11% <100.00%> (+0.83%)`	⬆️

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 e6bb4c4...23782b0. Read the comment docs.

jingtang10 · 2022-02-21T18:06:23Z

Thanks Kashyap for fixing this. First thought is that we should write some tests to cover this case. Upon second thought I'm not entirely sure why this would be the fix anyway - it seems like if val vs var can fix this issue there's probably an underlying problem which is the items are not being replaced/deleted/cached correctly.

joiskash · 2022-02-22T07:49:02Z

Thanks Kashyap for fixing this. First thought is that we should write some tests to cover this case. Upon second thought I'm not entirely sure why this would be the fix anyway - it seems like if val vs var can fix this issue there's probably an underlying problem which is the items are not being replaced/deleted/cached correctly.

Sure , ill write a UT first.

…emViewItem

jingtang10 · 2022-02-22T13:27:34Z

...capture/src/main/java/com/google/android/fhir/datacapture/views/QuestionnaireItemViewItem.kt

+ * perform a Diff before updating the view. While checking if two [QuestionnaireItemViewItem]s are
+ * equal or not, we need to check if they are referencing the same objects.
+ */
+ override fun equals(other: Any?): Boolean {


return early if other isn't also a questionnaire item view item?

...capture/src/main/java/com/google/android/fhir/datacapture/views/QuestionnaireItemViewItem.kt

jingtang10 · 2022-02-22T14:19:59Z

...capture/src/main/java/com/google/android/fhir/datacapture/views/QuestionnaireItemViewItem.kt

+ return this.questionnaireItem === (other as QuestionnaireItemViewItem).questionnaireItem &&
+ this.questionnaireResponseItem ===
+ (other as QuestionnaireItemViewItem).questionnaireResponseItem
+ }


add another function equalsDeep to handle the areContentsTheSame functino of the DiffCallback.

Co-authored-by: Jing Tang <[email protected]>

jingtang10

Thanks @joiskash again for debugging fixing this very thorny issue!

Just a few nits! Please feel free to merge once these are addressed.

jingtang10 · 2022-02-23T19:55:17Z

...capture/src/main/java/com/google/android/fhir/datacapture/views/QuestionnaireItemViewItem.kt

+ * [QuestionnaireResponseItem] match.
+ */
+ override fun equals(other: Any?): Boolean {
+ return if (other is QuestionnaireItemViewItem) {


turn this if condition around and return early.

jingtang10 · 2022-02-23T19:55:36Z

...capture/src/main/java/com/google/android/fhir/datacapture/views/QuestionnaireItemViewItem.kt

+ * On the other hand, under certain circumstances, the underlying [QuestionnaireResponseItem]
+ * might be recreated for the same question. For example, if a [QuestionnaireItem] is nested under
+ * another [QuestionnaireItem], the [QuestionnaireResponseItem](s) will be nested under the parent
+ * [QuestionnaireResponse.QuestionnaireResponseItemAnswerComponent]. and if the


Suggested change

* [QuestionnaireResponse.QuestionnaireResponseItemAnswerComponent]. and if the

* [QuestionnaireResponse.QuestionnaireResponseItemAnswerComponent], and if the

jingtang10 · 2022-02-23T19:56:59Z

...capture/src/main/java/com/google/android/fhir/datacapture/views/QuestionnaireItemViewItem.kt

+ * Comparing the contents of two [QuestionnaireItemViewItem]s,involves traversing the underlying
+ * [Questionnaire.QuestionnaireItemComponent] and
+ * [QuestionnaireResponse.QuestionnaireResponseItemComponent],and comparing values of all the
+ * properties. This is done by using the [Questionnaire.QuestionnaireItemComponent.equalsDeep] and
+ * [QuestionnaireResponse.QuestionnaireResponseItemComponent.equalsDeep] functions


Suggested change

* Comparing the contents of two [QuestionnaireItemViewItem]s,involves traversing the underlying

* [Questionnaire.QuestionnaireItemComponent] and

* [QuestionnaireResponse.QuestionnaireResponseItemComponent],and comparing values of all the

* properties. This is done by using the [Questionnaire.QuestionnaireItemComponent.equalsDeep] and

* [QuestionnaireResponse.QuestionnaireResponseItemComponent.equalsDeep] functions

* Comparing the contents of two [QuestionnaireItemViewItem]s by traversing the underlying

* [Questionnaire.QuestionnaireItemComponent] and

* [QuestionnaireResponse.QuestionnaireResponseItemComponent] and comparing values of all the

* properties. This is done by using [Questionnaire.QuestionnaireItemComponent.equalsDeep] and

* [QuestionnaireResponse.QuestionnaireResponseItemComponent.equalsDeep].

...capture/src/main/java/com/google/android/fhir/datacapture/views/QuestionnaireItemViewItem.kt

Co-authored-by: Jing Tang <[email protected]>

joiskash added 3 commits February 21, 2022 18:12

Call onAnswerChanged Callback instead of questionnaireResponseCHangeC…

5d56eaf

…allback

Remove unnecessary questionnaireResponseItemChangedCallback since onA…

9d355c2

…nswerChanged calls this internally

questionnaireResponseItem has to be a variable since we clear answers…

d1ca919

… which may contain nested questionnaireReponseItems

joiskash requested review from aditya-07, deepankarb, jingtang10, kevinmost, ktarasenko, stevenckngaa, vvikramraj and yigit as code owners February 21, 2022 12:55

Merge branch 'master' into kj/fix-single-option-drop-down

2ea44f3

joiskash added the Data capture label Feb 21, 2022

joiskash added 3 commits February 22, 2022 17:34

Reverted change from val to var and added override for equals function

4260e61

Changed areItemsTheSame to check internal objects

fac8525

Added UTs to check the overridden equals function for questionnaireIt…

d373bb5

…emViewItem

joiskash changed the title ~~Change questionnaireResponseItem to var from val in questionnaireItemViewItem~~ Change areItemsTheSame to check if internal objects are the same with === Feb 22, 2022

jingtang10 requested changes Feb 22, 2022

View reviewed changes

joiskash and others added 3 commits February 23, 2022 09:08

Update comments

74a40d6

Co-authored-by: Jing Tang <[email protected]>

Merge branch 'master' into kj/fix-single-option-drop-down

ac180c9

Add new equalsDeep function for questionnaireItemViewItem

f75f503

joiskash requested a review from jingtang10 February 23, 2022 13:38

jingtang10 approved these changes Feb 23, 2022

View reviewed changes

joiskash added 2 commits February 24, 2022 13:36

Early return and feedback in code comments

5414d82

Merge branch 'master' into kj/fix-single-option-drop-down

31ff47d

joiskash enabled auto-merge (squash) February 24, 2022 08:06

jingtang10 requested changes Feb 24, 2022

View reviewed changes

...capture/src/main/java/com/google/android/fhir/datacapture/views/QuestionnaireItemViewItem.kt Outdated Show resolved Hide resolved

Clean up if else structure

4409992

Co-authored-by: Jing Tang <[email protected]>

joiskash requested a review from jingtang10 February 24, 2022 10:32

SpotlessApply

23782b0

jingtang10 approved these changes Feb 24, 2022

View reviewed changes

joiskash merged commit f31f5b7 into master Feb 24, 2022

joiskash deleted the kj/fix-single-option-drop-down branch February 24, 2022 12:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change areItemsTheSame to check if internal objects are the same with === #1185

Change areItemsTheSame to check if internal objects are the same with === #1185

joiskash commented Feb 21, 2022 •

edited

Loading

codecov bot commented Feb 21, 2022 •

edited

Loading

jingtang10 commented Feb 21, 2022

joiskash commented Feb 22, 2022

jingtang10 Feb 22, 2022

joiskash Feb 23, 2022

jingtang10 Feb 22, 2022

joiskash Feb 23, 2022

jingtang10 left a comment •

edited

Loading

jingtang10 Feb 23, 2022

joiskash Feb 24, 2022

jingtang10 Feb 23, 2022

joiskash Feb 24, 2022

jingtang10 Feb 23, 2022

joiskash Feb 24, 2022

	* [QuestionnaireResponse.QuestionnaireResponseItemAnswerComponent]. and if the
	* [QuestionnaireResponse.QuestionnaireResponseItemAnswerComponent], and if the

Change areItemsTheSame to check if internal objects are the same with === #1185

Change areItemsTheSame to check if internal objects are the same with === #1185

Conversation

joiskash commented Feb 21, 2022 • edited Loading

codecov bot commented Feb 21, 2022 • edited Loading

Codecov Report

jingtang10 commented Feb 21, 2022

joiskash commented Feb 22, 2022

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 • 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

joiskash commented Feb 21, 2022 •

edited

Loading

codecov bot commented Feb 21, 2022 •

edited

Loading

jingtang10 left a comment •

edited

Loading