aurangzaibumer commented 2 years ago

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

Issue# 1272

Description Clear and concise code change description.

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

Type Choose one: Feature

Screenshots/Video

https://user-images.githubusercontent.com/35099184/170076662-ca522aa3-6ce5-48ed-96bc-85a65ec0938e.mp4

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

joiskash commented 2 years ago

Oh there is some weird bug that shows overlapped questions 0:44. Is there any reproducibility ? We will have to create an issue regarding this. But otherwise, looks great!

cc: @jingtang10

aurangzaibumer commented 2 years ago

@dubdabasoduba as per discussion on standup, there is a PR for submit button https://github.com/google/android-fhir/pull/1215 this needs to be merged first in order to work on the submit button so probably we can create a separate ticket for submit button implementation so this can be merged if reviewed.

codecov[bot] commented 2 years ago

Codecov Report

Merging #1301 (3e70103) into master (61c383e) will not change coverage. The diff coverage is n/a.

:exclamation: Current head 3e70103 differs from pull request most recent head 35b7774. Consider uploading reports for the commit 35b7774 to get more accurate results

@@      Coverage Diff       @@
##   master   #1301   +/-   ##
==============================
==============================

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

aurangzaibumer commented 2 years ago

Thinking about this a little more, I figured that this more than just validation.. This is triggering some logic on page change events within the QuestionnaireFragment. So we could refactor as such. This will allow more complex use cases such as :

Not allowing to switch page until a minimum amount of time has passed

Not allowing page change if the answers are exactly the same as the previous questionnaire answered . (this can happen in cases were there is a monetary incentive to complete forms)

Validation of all the items on the page can be the default implementation provided by the library. Developers can choose to add this in their Strategies too.

By having a context, the main point is that the strategy can be changed during run time. So a developer can do something like

dataConfig.questionnairePageEventContext.setStrategy(newStrategy)

at any point in the code during run time.

Create a context class that will be stored in the DataCapture Config
class QuestionnairePageChangeEventContext{
  private var pageChangeStrategy : PageChangeStrategy 

  fun setStrategy(pageChangeStrategy : PageChangeStrategy ){
      this.pageChangeStrategy = pageChangeStrategy
  }

  //This function will be called in the QuestionnaireViewModel goToNextPage and if returns true  then pageFlow.value will be changed.

  fun pageNextEvent( list: List<QuestionnaireItemViewItem> ):Boolean{
  return pageChangeStrategy.shouldGoToNextPage(list)
  }

  //This function will be called in the QuestionnaireViewModel goToPreviousPage and if returns true  then pageFlow.value will be changed.

  fun pagePreviousEvent( list: List<QuestionnaireItemViewItem> ):Boolean{
  return pageChangeStrategy.shouldGoToPreviousPage(list)
  }
}
The strategy interface will be like this.
interface PageChangeStrategy {

  fun shouldGoToPreviousPage(list: List<QuestionnaireItemViewItem>): Boolean

  fun shouldGoToNextPage(list: List<QuestionnaireItemViewItem>): Boolean

}
The default implementation can be like :
class DefaultPageChangeStrategy: PageChangeStrategy{

  override fun shouldGoToPreviousPage(list: List<QuestionnaireItemViewItem>): Boolean{
    return list.any { it.isErrorTriggered }
  }

  override fun shouldGoToNextPage(list: List<QuestionnaireItemViewItem>): Boolean
    return list.any { it.isErrorTriggered }
  }
and finally the change in the viewmodel can be as such:
internal class QuestionnaireViewModel(application: Application, state: SavedStateHandle) :{
  private val questionnairePageEventContext by lazy {
    DataCapture.getConfiguration(getApplication()).questionnairePageEventContext // declare and initialize this in the DataConfig. It should never be null
  }
.
.
.
  internal fun goToPreviousPage() {
    if (questionnairePageEventContext.pagePreviousEvent()){
              pageFlow.value = pageFlow.value!!.previousPage()
     }
  }

  internal fun goToNextPage() {
    if (questionnairePageEventContext.pageNextEvent()){
              pageFlow.value = pageFlow.value!!.nextPage()
     }
  }
}
Some one from google can comment on this, @jingtang @kevinmost

feedback implemented, please review @joiskash . Thanks

jingtang10 commented 2 years ago

@santosh-pingle can you leave some comments please

aurangzaibumer commented 2 years ago

Great work, left a few comments. I request someone from google and Ona to review this before you write the UTs. @jingtang10 @f-odhiambo

cc @dubdabasoduba

santosh-pingle commented 2 years ago

@santosh-pingle can you leave some comments please

sure.

aurangzaibumer commented 2 years ago

left few minor comments. Also please check whether we need demo/src/main/assets/new-patient-registration-paginated.json file change?

was checking the questionnaire for pagination. I'll revert it back. Thanks.

aurangzaibumer commented 2 years ago

Can you please review it again @santosh-pingle I had resolved the changes asked. Thanks

aurangzaibumer commented 2 years ago

Can you please review it again @santosh-pingle I had resolved the changes asked. Thanks

cc @dubdabasoduba

jingtang10 commented 2 years ago

can you rename the PR: Allow/disallow pagination based on validation result (e.g. can't go to next page with validation error)

jingtang10 commented 2 years ago

@aurangzaibumer @joiskash and I just had a discussion:

we should create new flows to control if the prev/next buttons are enabled or greyed out in the UI. these flows should exposed by the view model, and the UI class will collect these flows and update the buttons
the getQuestionnaireState method should probably incorporate questionnaire validation - at the moment it doesn't do any of that, it's probably incorrect. this also address @kevinmost 's todo he left in that function.
it's possible as a result of the previous item, we might not need to directly call validation code in the answer changed callback. all that needs to be done is to get a new questionnaire state. i'm not sure if this is true so please do test it out.

aurangzaibumer commented 2 years ago

the getQuestionnaireState method should probably incorporate questionnaire validation - at the moment it doesn't do any of that, it's probably incorrect. this also address @kevinmost 's todo he left in that function.

This is done, please review and share feedback

it's possible as a result of the previous item, we might not need to directly call validation code in the answer changed callback. all that needs to be done is to get a new questionnaire state. i'm not sure if this is true so please do test it out.

I have tried to remove the validation from onBind and onAnswerChanged but the sequence of the method calling is different as I expected. whenever you give any input, onAnswerChanged gets called first, after that getQuestionnaireState gets called hence we cannot use validationResult from the questionnaireViewItem model in the Method onAnswerChanged

aurangzaibumer commented 2 years ago

https://user-images.githubusercontent.com/35099184/170076662-ca522aa3-6ce5-48ed-96bc-85a65ec0938e.mp4

Case : Checked the validation of each items behind the pagination button logic.

pushed the latest changes please provide your feedback. thanks @jingtang10 @joiskash

aurangzaibumer commented 2 years ago

issue_1272.mp4 Case : Checked the validation of each items behind the pagination button logic.

pushed the latest changes please provide your feedback. thanks @jingtang10 @joiskash

removed some unnecessary code after reviewing old changes, pushed latest changes. Thanks

Tarun-Bhardwaj commented 2 years ago

@aurangzaibumer , is there an ETA you could provide so that @joiskash can continue with the code review? CC: @f-odhiambo

aurangzaibumer commented 2 years ago

@aurangzaibumer , is there an ETA you could provide so that @joiskash can continue with the code review? CC: @f-odhiambo

Discussed the issues/blockers with Jing & Kashyap today in the SDK call, Requested a discussion meeting by tomorrow or next week (on Jing's availability).

aurangzaibumer commented 1 year ago

As per discussion with Jing

He suggested to separate the Validation Changes from the Pagination changes so I'm removing the validation changes from this PR. This PR will only reflect changes that is related to pagination
The second thing He suggested is to use the ENUM approach instead of Page Change Strategy

Tarun-Bhardwaj commented 1 year ago

@aurangzaibumer @joiskash , is this PR blocked? Any support you need here to move forward?

aurangzaibumer commented 1 year ago

Made changes related to enum approach For now keeping

the validationResult code base
Paging change strategy will remove once discussion is done with @jingtang10 today

As per discussion below PR needs to be merged first in order to consume the validationResult code base from the QuestionnaireViewModel https://github.com/google/android-fhir/pull/1468

Tarun-Bhardwaj commented 1 year ago

@aurangzaibumer , could you please provide an ETA for addressing review comments from @joiskash ?

aurangzaibumer commented 1 year ago

@aurangzaibumer , could you please provide an ETA for addressing review comments from @joiskash ?

This current PR is dependent on https://github.com/google/android-fhir/pull/1468 because we will be using the validation logic from 1468 PR and remove it from here. Me and Jing are currently working on #1468 so we can close that ASAP

Tarun-Bhardwaj commented 1 year ago

@aurangzaibumer , considering that #1468 was merged last week, can the pending bits be closed on this PR as well? CC: @f-odhiambo @jingtang10

aurangzaibumer commented 1 year ago

@aurangzaibumer , considering that #1468 was merged last week, can the pending bits be closed on this PR as well? CC: @f-odhiambo @jingtang10

I'm facing an issue, for suppose when we open the questionnaire there are no errors shown and when user taps on next page (on restrict mode), the application should be showing errors on required fields (if not entered) in this case we have to re-validate the questionnaire (even if they are not being modified by the user) and also have to reflect the errors (where applicable) on the UI. Now what I did was that I iterate over currentPageItems so each element could have latest validationResult (because there might be a case where fields are not being modified by the user yet) and then I have to update the UI in-order to reflect the errors

 currentPageItems.forEach {
         it.validationResult?.isValid = QuestionnaireResponseItemValidator.validate(
           it.questionnaireItem,
           it.answers,
           this@QuestionnaireViewModel.getApplication()
         ).isValid

       }

can you please share your opinion if that's the right strategy. @jingtang10

cc @maimoonak @joiskash

jingtang10 commented 1 year ago

@aurangzaibumer , considering that #1468 was merged last week, can the pending bits be closed on this PR as well? CC: @f-odhiambo @jingtang10

I'm facing an issue, for suppose when we open the questionnaire there are no errors shown and when user taps on next page (on restrict mode), the application should be showing errors on required fields (if not entered) in this case we have to re-validate the questionnaire (even if they are not being modified by the user) and also have to reflect the errors (where applicable) on the UI. Now what I did was that I iterate over currentPageItems so each element could have latest validationResult (because there might be a case where fields are not being modified by the user yet) and then I have to update the UI in-order to reflect the errors
 currentPageItems.forEach {
         it.validationResult?.isValid = QuestionnaireResponseItemValidator.validate(
           it.questionnaireItem,
           it.answers,
           this@QuestionnaireViewModel.getApplication()
         ).isValid

       }
can you please share your opinion if that's the right strategy. @jingtang10

cc @maimoonak @joiskash

can you add all the items to the modified set when the user tries to go to the next page?

aurangzaibumer commented 1 year ago

@aurangzaibumer , considering that #1468 was merged last week, can the pending bits be closed on this PR as well? CC: @f-odhiambo @jingtang10

I'm facing an issue, for suppose when we open the questionnaire there are no errors shown and when user taps on next page (on restrict mode), the application should be showing errors on required fields (if not entered) in this case we have to re-validate the questionnaire (even if they are not being modified by the user) and also have to reflect the errors (where applicable) on the UI. Now what I did was that I iterate over currentPageItems so each element could have latest validationResult (because there might be a case where fields are not being modified by the user yet) and then I have to update the UI in-order to reflect the errors
 currentPageItems.forEach {
         it.validationResult?.isValid = QuestionnaireResponseItemValidator.validate(
           it.questionnaireItem,
           it.answers,
           this@QuestionnaireViewModel.getApplication()
         ).isValid

       }
can you please share your opinion if that's the right strategy. @jingtang10 cc @maimoonak @joiskash
can you add all the items to the modified set when the user tries to go to the next page?

I have used a boolean that will be set as true whenever next page is pressed as

isPaginationButtonPressed = true modificationCount.update { it + 1 }

and updated a check on questionnareState as

modifiedQuestionnaireResponseItemSet.contains(questionnaireResponseItem) || isPaginationButtonPressed

and it will be reset to false before pageFlow value changes as

if (currentPageItems.none { !it.validationResult!!.isValid }) { isPaginationButtonPressed = false pageFlow.value = pageFlow.value!!.nextPage() }

https://user-images.githubusercontent.com/35099184/180257590-020eeeb3-ec79-4333-9c56-9ce2a968aef0.mp4

can you please review if it's a better strategy @jingtang10

cc @maimoonak @f-odhiambo

jingtang10 commented 1 year ago

Thanks umer - this is much better

aurangzaibumer commented 1 year ago

Thanks umer - this is much better

thanks @jingtang10 , I'll be updating the documentation and the test cases based on this implementation.

cc @f-odhiambo @maimoonak

jingtang10 commented 1 year ago

please revert the license header changes - makes it very difficult to review the code.

thanks!

aurangzaibumer commented 1 year ago

All comments are resolved. Can you please review and share you feedback @jingtang10 Thanks

aurangzaibumer commented 1 year ago

Wow this PR looks so clean! Great work, just a small comment, we should probably add a test that checks the default entry mode if there is no extension for entry mode in the questionnaire.

added a case when no entryMode is defined in the questionnaire. Thanks

aurangzaibumer commented 1 year ago

@jingtang10 can you please review on your availability. thanks

santosh-pingle commented 1 year ago

@aurangzaibumer can you please add some questionnaire examples?

aurangzaibumer commented 1 year ago

@aurangzaibumer can you please add some questionnaire examples?

"extension": [ 
    {
      "url" : "http://hl7.org/fhir/uv/sdc/StructureDefinition/sdc-questionnaire-entryMode",
      "valueCode" : "prior-edit"
    } 
 ]

for more entry-mode codes : http://build.fhir.org/ig/HL7/sdc/ValueSet-entryMode.html

google / android-fhir

Implement the `entryMode` extension #1301

Codecov Report