maimoonak commented 2 years ago

Fixes #971 #1173

Description Implemented the calculated expression extension. The feature also calculated cyclic dependency, and prevents the infinite calls of callback

Type Choose one: Feature

Checklist

[x] I have read and acknowledged the Code of conduct.
[x] I have read the Contributing page.
[x] I have signed the Google Individual CLA, or I am covered by my company's Corporate CLA.
[x] I have discussed my proposed solution with code owners in the linked issue(s) and we have agreed upon the general approach.
[x] I have run ./gradlew spotlessApply and ./gradlew spotlessCheck to check my code follows the style guide of this project.
[x] I have run ./gradlew check and ./gradlew connectedCheck to test my changes locally.
[x] 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).

f-odhiambo commented 2 years ago

@RaaziaTarique kindly take a 1st pass review

codecov[bot] commented 2 years ago

Codecov Report

Merging #1380 (6c2e0e4) into master (da1b5b3) will increase coverage by 32.61%. The diff coverage is 14.07%.

:exclamation: Current head 6c2e0e4 differs from pull request most recent head 86e7a2b. Consider uploading reports for the commit 86e7a2b to get more accurate results

@@              Coverage Diff              @@
##             master    #1380       +/-   ##
=============================================
+ Coverage          0   32.61%   +32.61%     
- Complexity        0      320      +320     
=============================================
  Files             0      151      +151     
  Lines             0     5273     +5273     
  Branches          0      942      +942     
=============================================
+ Hits              0     1720     +1720     
- Misses            0     3307     +3307     
- Partials          0      246      +246

Impacted Files	Coverage Δ
...hir/datacapture/MoreQuestionnaireItemComponents.kt	`14.28% <0.00%> (ø)`
...re/MoreQuestionnaireResponseItemAnswerComponent.kt	`0.00% <0.00%> (ø)`
...android/fhir/datacapture/QuestionnaireViewModel.kt	`0.00% <0.00%> (ø)`
...d/fhir/datacapture/fhirpath/ExpressionEvaluator.kt	`0.00% <0.00%> (ø)`
...android/fhir/datacapture/mapping/ResourceMapper.kt	`0.00% <0.00%> (ø)`
...roid/fhir/datacapture/validation/ValidationUtil.kt	`33.33% <0.00%> (ø)`
...ws/QuestionnaireItemDatePickerViewHolderFactory.kt	`62.90% <50.00%> (ø)`
...stionnaireItemEditTextQuantityViewHolderFactory.kt	`73.07% <86.66%> (ø)`
...droid/fhir/db/impl/entities/DateTimeIndexEntity.kt	`100.00% <0.00%> (ø)`
...fhir/search/filter/QuantityParamFilterCriterion.kt	`100.00% <0.00%> (ø)`
... and 149 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Tarun-Bhardwaj commented 2 years ago

Assigning to @aditya-07 for review.

maimoonak commented 2 years ago

@jingtang10 @aditya-07 Please review the changes

Tarun-Bhardwaj commented 1 year ago

@aditya-07 @jingtang10 could you please review the changes?

jingtang10 commented 1 year ago

@joiskash can you please leave a review? thanks!

shelaghm commented 1 year ago

Can you attach a screenshot please?

@shelaghm not sure if we want to show case this in the layout screen of the catalog app... i kind feel like this is better off in the components page... that is if we want to showcase this at all.

I agree that it's likely more useful to show in components page, not layout page. Having a screenshot that I can look at of this components will he helpful for me to provide more specific guidance.

Tarun-Bhardwaj commented 1 year ago

@maimoonak could you please advise an ETA by when these changes can be ready for review?

maimoonak commented 1 year ago

@Tarun-Bhardwaj I have resolved all conflicts with main, synced the code with new changes as well just now. The issue https://github.com/google/android-fhir/issues/1498 needs to be resolved to run the catalog (without extra needless edit-text). But this PR and functionality does not depend on this issue and is working as expected. (Below is a working example)

ezgif com-gif-maker

Tarun-Bhardwaj commented 1 year ago

@maimoonak , could you please advise the status of these changes? Is there an ETA by when we can merge this PR?

maimoonak commented 1 year ago

@Tarun-Bhardwaj @jingtang10 kindly review the PR.

jingtang10 commented 1 year ago

can you please make sure to reply to the review comments and mark the conversations as resolved if the comment is addressed in the code. once that's done please reassign back to me. thanks

jingtang10 commented 1 year ago

@maimoonak please reassign to me once all comments are addressed

maimoonak commented 1 year ago

@jingtang10 I have found few libraries for graph generation. I am still exploring these and if its possible to use any for our expression dependency graph https://github.com/systek/dataflow https://github.com/jgrapht/jgrapht/ https://github.com/google/guava/wiki/GraphsExplained https://github.com/nidi3/graphviz-java#user-content-api

jingtang10 commented 1 year ago

@jingtang10 I have found few libraries for graph generation. I am still exploring these and if its possible to use any for our expression dependency graph https://github.com/systek/dataflow https://github.com/jgrapht/jgrapht/ https://github.com/google/guava/wiki/GraphsExplained https://github.com/nidi3/graphviz-java#user-content-api

Thanks Maimoona - what I had in mind was just to use a basic data structure to represent the dependencies. I wasn't sure that we'd need to use any external libraries for this.

We already have some auxiliary data strcuture such as the parent map in the view model - this would be similar to that I thought.

jingtang10 commented 1 year ago

@maimoonak i turned on auto-merge. so if you update the branch it should merge automatically.

thanks

google / android-fhir

Implement calculated-expression extension #1380

Codecov Report