1272 Allow/disallow pagination based on validation result (e.g. can't go to next page with validation error)

[aurangzaibumer]

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]

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]

@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]]

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]

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 :

1. Not allowing to switch page until a minimum amount of time has passed
2. 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]

@santosh-pingle can you leave some comments please

[aurangzaibumer]

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]

@santosh-pingle can you leave some comments please

sure.

[aurangzaibumer]

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]

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

[aurangzaibumer]

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

cc @dubdabasoduba

[jingtang10]

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

[jingtang10]

@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]

• 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]

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]

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]

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

[aurangzaibumer]

@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]

As per discussion with Jing

1. 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
2. The second thing He suggested is to use the ENUM approach instead of Page Change Strategy

[Tarun-Bhardwaj]

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

[aurangzaibumer]

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]

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

[aurangzaibumer]

@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]

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

[aurangzaibumer]

@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,
           [email protected]()
         ).isValid

       }

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

cc @maimoonak @joiskash

[jingtang10]

@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,
           [email protected]()
         ).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]

@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,
           [email protected]()
         ).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]

Thanks umer - this is much better

[aurangzaibumer]

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]

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

thanks!

[aurangzaibumer]

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