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

Evaluating variables in answerExpressions #2066

Merged
merged 6 commits into from
Jul 23, 2023
Merged

Conversation

MJ1998
Copy link
Collaborator

@MJ1998 MJ1998 commented Jul 7, 2023

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

Fixes #2052

Description
There are 2 errors:

  1. answerExpressionMap incorrectly caches expressions
  2. variables in x-fhir-query are not supported, only launchContext

Solution here is to

  1. refactor to cache only when the full xFhirExpressionString has been constructed
  2. evaluate variables and their values in fhir-query expression

Alternative(s) considered
Removing caching of answer options but that makes it slow.

Type
Bug fix | Feature

Screenshots (if applicable)
[Used questionnaire json given in the issue linked]
Screenshot 2023-07-07 at 8 09 33 PM

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

@MJ1998 MJ1998 requested review from a team and santosh-pingle as code owners July 7, 2023 14:40
@MJ1998 MJ1998 requested a review from jingtang10 July 7, 2023 14:40
Copy link
Collaborator

@FikriMilano FikriMilano left a comment

Choose a reason for hiding this comment

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

Really nice code! @MJ1998
Just some minor comments.

@FikriMilano
Copy link
Collaborator

@MJ1998 this looks good to me, it will be nice to also merge this to master.

We can perhaps have this enhancement in future PRs?
#2034 (comment)
#2052 (comment)

But from the other PR's #2034 (comment) you tagged me, it seems its better to modify the QuestionnaireViewItem API with enabledAnswerOptionList .

@FikriMilano
Copy link
Collaborator

@jingtang10 kindly review

FikriMilano added a commit to opensrp/android-fhir that referenced this pull request Jul 18, 2023
@omarismail94 omarismail94 self-assigned this Jul 18, 2023
Copy link
Contributor

@omarismail94 omarismail94 left a comment

Choose a reason for hiding this comment

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

Nice change!

@omarismail94 omarismail94 merged commit b8836f8 into google:master Jul 23, 2023
1 of 2 checks passed
jingtang10 pushed a commit that referenced this pull request Feb 27, 2024
* Add X-FHIR-Query support for variable extension

* Add unit tests

* Fix dynamic answerExpression based on #2066

* Address review
- Use viewModelScope for calculated-expression
- Move the runBlocking to QuestionnaireItemViewItem for cqf-expression
- Rename questionnaireVariablesMap to questionnaireVariableMap

* Makes sure initial value got emitted to the questionnaireStateFlow

Notifies the flow which will update the data stream when updateDependentQuestionnaireResponseItems finished running

* Notify to not delay the update of UI

- updateDependentQuestionnaireResponseItems can evaluate
  X-FHIR-Query expression which can delay the update of UI
  for 1-2 seconds depending on the retrieved resource size.

- In the case of clicking next page after selecting an
  answer that shows the next pages based on skip logic
  (tapping this and that very quickly). We notify the UI
  pages immediately, then once the X-FHIR-Query expression
  is processed (1-2 seconds), we notify the UI again.
  Without this changes, when the user taps next page, the
  next pages that is supposed to be shown by skip logic
  will be shown late enough that the user don't see it,
  but if the user taps previous page button, user will see
  the shown pages decided by skip logic. And that looks
  like a bug.

* WIP

* Adjust code after major merge conflict

* spotlessApply

* Fix test

* Fix test

* Integrate suspend functions

Related to enable when expressions.

* spotlessApply

* Add missing xFhirQueryResolver param

* Address review

* Address review and refactor to use suspend function

* Run suspend for Barcode

* Address review

* Remove hasMissingParamValue

Better alternative is to limit the received
resource by using the _count common
parameter.

Example:
Patient?name=&_count=2

* Fix test

* Update QuestionnaireViewItemTest

* Use runTest in QuestionnaireUiEspressoTest

* Add missing import to access child of ChipGroup

* spotlessApply

* Fix test

* Add missing coroutine launch

* spotlessApply
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet