Allow using a custom TransformSupportServices for StructureMap-based extraction #712

ekigamba · 2021-08-16T07:54:09Z

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

Allow using a custom TransformSupportServices for StructureMap. When using StructureMap-based extraction, we use the command create('ResourceType') to generate new instances of resources. The current implementation lacks support for internal/embedded model types eg RiskAssessment.Prediction, Immunization.Reaction. Here is the usage https://github.com/opensrp/fhircore/blob/issue/403-refactor-extraction-code/android/engine/src/main/java/org/smartregister/fhircore/engine/helper/TransformSupportServices.kt#L50

Description
Add an optional TransformSupportServices param to the ResourceMapper#extract method and ResourceMapper#extractByStructureMap

Alternative(s) considered
Yes, adding support for the internal models inside the datacapture module. This would take longer to implement because the number of models lacking support is not clear. Adding support for an internal models would also require making changes to android-fhir.

Type
Choose one: Feature

Checklist

I have read and acknowledged the Code of conduct
I have read How to Contribute
I have read the Developer's guide
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)

codecov-commenter · 2021-08-16T08:17:51Z

Codecov Report

Merging #712 (8aa3aaa) into master (1ee392e) will increase coverage by 0.03%.
The diff coverage is 82.35%.

@@             Coverage Diff              @@
##             master     #712      +/-   ##
============================================
+ Coverage     84.53%   84.57%   +0.03%     
- Complexity      646      651       +5     
============================================
  Files           140      140              
  Lines         10488    10494       +6     
  Branches        795      795              
============================================
+ Hits           8866     8875       +9     
+ Misses         1223     1220       -3     
  Partials        399      399

Impacted Files	Coverage Δ
...ogle/android/fhir/datacapture/DataCaptureConfig.kt	`83.33% <ø> (ø)`
...android/fhir/datacapture/mapping/ResourceMapper.kt	`86.73% <75.00%> (-0.25%)`	⬇️
...tacapture/mapping/StructureMapExtractionContext.kt	`100.00% <100.00%> (ø)`
...com/google/android/fhir/datacapture/DataCapture.kt	`83.33% <0.00%> (+16.66%)`	⬆️

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 1ee392e...8aa3aaa. Read the comment docs.

ekigamba · 2021-08-17T08:16:12Z

@jingtang10 Kindly review. This is a small PR that fixes an issue with ResourceMapper extraction using StructureMap-based extraction. The issue came up while trying to implement a StructureMap for patient registration

…-support-services

Tarun-Bhardwaj · 2022-01-29T11:04:33Z

@ekigamba , is there an ETA by when the changes can be committed to make the PR ready for review?

aditya-07

A few comments, otherwise the PR looks good.

aditya-07 · 2022-01-31T14:31:24Z

datacapture/src/main/java/com/google/android/fhir/datacapture/mapping/ResourceMapper.kt

- else extractByStructureMap(context, questionnaire, questionnaireResponse, structureMapProvider)
+ else
+ extractByStructureMap(
+ context,


Since context is also just used for structure map based extraction, maybe it should also be encapsulated inside StructureMapExtractionContext.

aditya-07 · 2022-01-31T14:32:55Z

...e/src/main/java/com/google/android/fhir/datacapture/mapping/StructureMapExtractionContext.kt

+ val structureMapProvider: (suspend (String, IWorkerContext) -> StructureMap?),
+ val transformSupportServices: StructureMapUtilities.ITransformerServices? = null


I think reversing these would be better for creating StructureMapExtractionContext objects with lambda.

aditya-07 · 2022-02-02T05:40:11Z

datacapture/src/test/java/com/google/android/fhir/datacapture/mapping/ResourceMapperTest.kt

+ iParser.parseResource(QuestionnaireResponse::class.java, questionnaireResponseJson) as
+ QuestionnaireResponse
+
+ val outputs: MutableList<Base> = mutableListOf()


Since outputs is not being used anywhere, can it be avoided altogether?

- Disable TransformSupportServices required for extraction - Re-add the validation and optimize the Q & QR copy - Pass Q and QR as objects to QuestionnaireFragment instead of encoding and decoding again - Fix breaking changes due to missing PRs google/android-fhir#712 and google/android-fhir#807

jingtang10

Please address aditya's comments.

@ekigamba - thanks for this pr and the good work in creating the tests. i think the knowledge and experience to create structure map from a mapping is pretty lacking in general (i have no idea how to do this). can you please write a little bit of documentation somewhere and we can put it on the wiki?

thanks!

jingtang10 · 2022-02-07T17:38:20Z

datacapture/src/main/java/com/google/android/fhir/datacapture/mapping/ResourceMapper.kt

- StructureMapUtilities(simpleWorkerContext)
+ StructureMapUtilities(
+ simpleWorkerContext,
+ structureMapExtractionContext.transformSupportServices


i went into hapi's code for a quick look, can't really figure out what they do in the case of this being null. just to be safe, shouldn't we call different contructors if this is null or not?

I confirmed that that both constructors have the same logic with one having the setting for the TransformSupportServices

https://github.com/hapifhir/org.hl7.fhir.core/blob/master/org.hl7.fhir.r4/src/main/java/org/hl7/fhir/r4/utils/StructureMapUtilities.java#L253..L259

https://github.com/hapifhir/org.hl7.fhir.core/blob/master/org.hl7.fhir.r4/src/main/java/org/hl7/fhir/r4/utils/StructureMapUtilities.java#L261..L266

ekigamba · 2022-02-17T08:35:24Z

@jingtang10 I will add some documentation on the conversion

…-support-services

aditya-07

Please check the comment. Apart from that LGTM

aditya-07 · 2022-02-28T08:40:10Z

...e/src/main/java/com/google/android/fhir/datacapture/mapping/StructureMapExtractionContext.kt

+ val transformSupportServices: StructureMapUtilities.ITransformerServices? = null,
+ val context: Context,


nit: Can you please swap these? Context is usually the first param.

…-support-services

ekigamba requested review from aditya-07, deepankarb, jingtang10, joiskash, kevinmost, pankajkatoch, stevenckngaa and yigit as code owners August 16, 2021 07:54

ekigamba marked this pull request as draft August 16, 2021 07:54

Allow using a custom TransformSupportServices for StructureMap

1ad28bb

ekigamba force-pushed the ek/resource-mapper-adding-custom-transform-support-services branch from 295e871 to 1ad28bb Compare August 16, 2021 07:56

ekigamba self-assigned this Aug 16, 2021

ekigamba changed the title ~~Hotfix: Allow using a custom TransformSupportServices for StructureMap~~ Allow using a custom TransformSupportServices for StructureMap-based extraction Aug 16, 2021

ekigamba marked this pull request as ready for review August 16, 2021 22:15

ekigamba assigned jingtang10 and unassigned ekigamba Aug 16, 2021

Merge branch 'master' into ek/resource-mapper-adding-custom-transform…

52a7e3d

…-support-services

ekigamba assigned aditya-07 Sep 16, 2021

Merge branch 'master' into ek/resource-mapper-adding-custom-transform…

81fab86

…-support-services

Tarun-Bhardwaj added this to In progress in Data capture library via automation Sep 20, 2021

ekigamba added 3 commits October 21, 2021 10:32

Merge branch 'master' into ek/resource-mapper-adding-custom-transform…

5fbcc29

…-support-services

Merge branch 'master' into ek/resource-mapper-adding-custom-transform…

85f14ed

…-support-services

Merge branch 'master' into ek/resource-mapper-adding-custom-transform…

4ea1899

…-support-services

ekigamba removed the request for review from pankajkatoch November 10, 2021 14:17

ekigamba added 3 commits November 12, 2021 15:59

Merge branch 'master' into ek/resource-mapper-adding-custom-transform…

a3a926d

…-support-services

Fix compile error in ResourceMapperTest

6542e93

Add ResourceMapper#extract test with custom TransformSupportServices

0b1938d

ekigamba force-pushed the ek/resource-mapper-adding-custom-transform-support-services branch from 0a8111c to 0b1938d Compare November 17, 2021 14:03

ekigamba and others added 4 commits January 13, 2022 11:31

Cleanup code

13689b2

Merge branch 'master' into ek/resource-mapper-adding-custom-transform…

be17baf

…-support-services

Cleanup code

8d76320

Merge branch 'master' into ek/resource-mapper-adding-custom-transform…

c3981fe

…-support-services

ekigamba requested a review from aditya-07 January 14, 2022 07:10

aditya-07 reviewed Jan 31, 2022

View reviewed changes

aditya-07 reviewed Feb 2, 2022

View reviewed changes

jingtang10 reviewed Feb 7, 2022

View reviewed changes

jingtang10 assigned ekigamba and unassigned aditya-07 and jingtang10 Feb 8, 2022

Cleanup StructureMapExtractionContext

c767dbe

ekigamba requested a review from ktarasenko as a code owner February 17, 2022 09:06

ekigamba and others added 2 commits February 17, 2022 12:07

Merge branch 'master' into ek/resource-mapper-adding-custom-transform…

7aea554

…-support-services

Merge branch 'master' into ek/resource-mapper-adding-custom-transform…

07c40ad

…-support-services

ekigamba requested review from aditya-07 and jingtang10 February 18, 2022 09:34

Merge branch 'master' into ek/resource-mapper-adding-custom-transform…

b32d66b

…-support-services

ekigamba requested a review from vvikramraj as a code owner February 24, 2022 08:42

aditya-07 approved these changes Feb 28, 2022

View reviewed changes

Merge branch 'master' into ek/resource-mapper-adding-custom-transform…

a258231

…-support-services

ekigamba requested a review from omarismail94 as a code owner February 28, 2022 12:04

Code cleanup

8aa3aaa

ekigamba merged commit 6aff204 into master Feb 28, 2022

Data capture library automation moved this from In progress to Done Feb 28, 2022

ekigamba deleted the ek/resource-mapper-adding-custom-transform-support-services branch February 28, 2022 15:33

fredhersch added the Data capture label Aug 29, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow using a custom TransformSupportServices for StructureMap-based extraction #712

Allow using a custom TransformSupportServices for StructureMap-based extraction #712

ekigamba commented Aug 16, 2021 •

edited

Loading

codecov-commenter commented Aug 16, 2021 •

edited by codecov bot

Loading

ekigamba commented Aug 17, 2021

Tarun-Bhardwaj commented Jan 29, 2022

aditya-07 left a comment

aditya-07 Jan 31, 2022

ekigamba Feb 17, 2022

aditya-07 Jan 31, 2022

ekigamba Feb 17, 2022

aditya-07 Feb 2, 2022

ekigamba Feb 17, 2022

jingtang10 left a comment

jingtang10 Feb 7, 2022

ekigamba Feb 17, 2022

ekigamba commented Feb 17, 2022

aditya-07 left a comment

aditya-07 Feb 28, 2022

ekigamba Feb 28, 2022

		val structureMapProvider: (suspend (String, IWorkerContext) -> StructureMap?),
		val transformSupportServices: StructureMapUtilities.ITransformerServices? = null

		val transformSupportServices: StructureMapUtilities.ITransformerServices? = null,
		val context: Context,

Allow using a custom TransformSupportServices for StructureMap-based extraction #712

Allow using a custom TransformSupportServices for StructureMap-based extraction #712

Conversation

ekigamba commented Aug 16, 2021 • edited Loading

codecov-commenter commented Aug 16, 2021 • edited by codecov bot Loading

Codecov Report

ekigamba commented Aug 17, 2021

Tarun-Bhardwaj commented Jan 29, 2022

aditya-07 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

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

ekigamba commented Feb 17, 2022

aditya-07 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekigamba commented Aug 16, 2021 •

edited

Loading

codecov-commenter commented Aug 16, 2021 •

edited by codecov bot

Loading