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.

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

enableWhenExpression follow up PR for maintaining CodeHealth #1369

Conversation

RaaziaTarique
Copy link
Contributor

@RaaziaTarique RaaziaTarique commented May 6, 2022

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

Fixes #1345

Description
Remove questionnaireResponseItemRetriever and instead implement the functionality of finding the questionnaire response item from linkId inside of the enablement package.

Alternative(s) considered
Have you considered any alternatives? And if so, why have you chosen the approach in this PR?

Type
Choose one: Code health

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

@codecov
Copy link

codecov bot commented May 9, 2022

Codecov Report

Merging #1369 (ab375c1) into master (472aec4) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master    #1369      +/-   ##
============================================
+ Coverage     85.65%   85.67%   +0.01%     
+ Complexity      713      709       -4     
============================================
  Files           149      149              
  Lines         10746    10745       -1     
  Branches        855      854       -1     
============================================
+ Hits           9205     9206       +1     
+ Misses         1104     1103       -1     
+ Partials        437      436       -1     
Impacted Files Coverage Δ
...hir/datacapture/MoreQuestionnaireItemComponents.kt 85.04% <100.00%> (+2.43%) ⬆️
...android/fhir/datacapture/QuestionnaireViewModel.kt 76.47% <100.00%> (-1.73%) ⬇️
...fhir/datacapture/enablement/EnablementEvaluator.kt 81.25% <100.00%> (+1.25%) ⬆️
...va/com/google/android/fhir/db/impl/DatabaseImpl.kt 91.34% <0.00%> (+0.96%) ⬆️

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 472aec4...ab375c1. Read the comment docs.

@@ -164,3 +150,25 @@ val fhirPathEngine: FHIRPathEngine =
with(FhirContext.forCached(FhirVersionEnum.R4)) {
FHIRPathEngine(HapiWorkerContext(this, DefaultProfileValidationSupport(this)))
}

/** Map from link IDs to questionnaire response items. */
private fun linkIdToQuestionnaireResponseItemMap(questionnaireResponse: QuestionnaireResponse) =
Copy link
Collaborator

Choose a reason for hiding this comment

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

there is a similar method/map in QuestionnaireViewModel (linkIdToQuestionnaireResponseItemMap). We should utilize that. This map creation every time for each evaluation would be expensive. Or is there any specific reason to not use that map!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We were previously using the same mapping but it was being done in the QuestionnaireViewModel as we were not passing the QuestionnaireResponse to EnablementEvaluator. I can use the same method present QuestionnaireViewModel though.

@RaaziaTarique RaaziaTarique requested a review from maimoonak May 10, 2022 09:44
@RaaziaTarique RaaziaTarique changed the title Rt/1345 enable when expression follow up enableWhenExpression follow up PR for maintaining CodeHealth May 12, 2022
@jingtang10
Copy link
Collaborator

@joiskash please take a look at this. We spoke about this before. This is a really important refactoring.

Copy link
Collaborator

@joiskash joiskash left a comment

Choose a reason for hiding this comment

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

These changes look good, but I am unable to figure out one part:

In questionnaireResponseItemChangedCallback the linkIdToQuestionnaireResponseItemMap is modified. Is this map equivalent to the map that you get after createLinkIdToQuestionnaireResponseItemMap with the updated questionnaireResponse ?

@RaaziaTarique @jingtang10

@RaaziaTarique
Copy link
Contributor Author

RaaziaTarique commented May 18, 2022

These changes look good, but I am unable to figure out one part:

In questionnaireResponseItemChangedCallback the linkIdToQuestionnaireResponseItemMap is modified. Is this map equivalent to the map that you get after createLinkIdToQuestionnaireResponseItemMap with the updated questionnaireResponse ?

@RaaziaTarique @jingtang10

Yes, as I have moved createLinkIdToQuestionnaireResponseItemMap to MoreQuestionnaireItemComponent so, I modified it as per EnablementEvaluator.
@joiskash CC: @jingtang10 @f-odhiambo

@Tarun-Bhardwaj
Copy link

@f-odhiambo are you aware if this PR is ready for review by Google?

@f-odhiambo
Copy link
Collaborator

@f-odhiambo are you aware if this PR is ready for review by Google?

This PR is ready for review @Tarun-Bhardwaj

@Tarun-Bhardwaj Tarun-Bhardwaj added the P2 Medium priority issue label Jun 27, 2022
@jingtang10
Copy link
Collaborator

i'll raise a separate pr

@jingtang10 jingtang10 closed this Jul 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2 Medium priority issue
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Remove questionnaireResponseItemRetriever
6 participants