Migrating Validation Result logic from View factory to QuestionnaireViewModel

[aurangzaibumer]

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

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: Improvement

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

[aurangzaibumer]

@jingtang10 can you please review. As discussed last week I had created a separate PR for the validationResult changes, removed validation being instantiated on View Factory.

[joiskash]

Looks good to me! Just a small comment, Is there a unit test you can write to verify that validation is takes place in getQuestionnaireState function?

[aurangzaibumer]

Looks good to me! Just a small comment, Is there a unit test you can write to verify that validation is takes place in getQuestionnaireState function?

Did update one of the unit tests where list of questionnaireItemViewItem is getting generated

[jingtang10]

please fix the failing tests

[aurangzaibumer]

I tried to fix some of the unit tests by triggering validation for the first time in the view factory* (in bind method), the problem is we need to fix #1427 first so on each interaction (when onAnswerChanged calls) there will be a new list of questionnaireItemViewItem along with the validationResult that gets generated by the View Model (in the getQuestionnaireState method) and we need to refresh the list so new validationResult can be reflected on the views. @jingtang10 @f-odhiambo

[joiskash]

With the patch in #1486 collect gets triggered as expected. The two issues raised by @aurangzaibumer are , please add any details that I missed:

1. How do we trigger the validation result to be displayed? Currently since the QuestionnaireItemViewItem is refers to the questionnaireResponse, the diffUtil will return a true for areItemsSame and areContentsSame.
2. Since we create new questionnaireItemViewItems the modified field gets overwritten.

cc : @jingtang10

[aurangzaibumer]

With the patch in #1486 collect gets triggered as expected. The two issues raised by @aurangzaibumer are , please add any details that I missed:

1. How do we trigger the validation result to be displayed? Currently since the QuestionnaireItemViewItem is refers to the questionnaireResponse, the diffUtil will return a true for areItemsSame and areContentsSame.
2. Since we create new questionnaireItemViewItems the modified field gets overwritten.

cc : @jingtang10

Thanks @joiskash, you have covered everything. So these are the issues i'm currently facing which is kind of a blocker to #1301 as well because that PR needs a validation in order to work for a PRIOR_EDIT Pagination (one of the entry mode). So we need to keep the state of the modified field and to reflect the ValidationResult (that was captured from the QuestionnaireViewModel) to the UI level without refreshing the whole screen

[jingtang10]

This turned out to be major rewrite of the library.

In order for questionnaire item comparison to actually work, the way we create QuestionnaireItemViewItems has been changed.

See documentation in the QuestionnaireItemViewItem class. The questionnaireResponseItem is marked as private so that views should not hav access to it. Instead, the views should use the APIs provided to update the answers.

The ansers are then used for comparison with what's already in the questionnaire to decide how the recycler view UI should be updated.

This results in changes in the DiffCallback implementation in the QuestionnaireItemAdapter.kt file. We consider two items to be the same if they're pointing to the same questionnaire item and questionnaire response item. But we will consider the contents to be the same only if the answers and validation results are also the same. These are the actual dynamic data during user interaction with the UI.

[omarismail94]

Initial feedback

[omarismail94]

This change is 👌👌👌👌👌👌👌👌👌

[maimoonak]

@jingtang10 I could not figure out the recursion here when finding the item by linkId. Could you please confirm that this method would find the item even at 10th level inside hierarchy.

item[index]: {
  linkId: 1
  answer.value -> value
  answer.item -> list of another nested 'item' with answer and items
}

So lets say inside linkId:1 there is item linkId=1.1.1.1 at fourth level .. would it find it with this method?

[jingtang10]

all this function does is to look for the first item (DFS) in the questionnaire response with a certain link id. it doesn't concat the string or anything.

[jingtang10]

see #1496

[shoaibmushtaq25]

@jingtang10 , I think @maimoonak wanted to add a comment on this method.
Here we are checking only one level down in the descendants but there is no recursion to check for all the levels down in the hierarchy (Let's say if we have 10 levels of nesting). What do you think about it?

[jingtang10]

ah you're right.

will send a fix.

[shoaibmushtaq25]

okay, Thanks.

[jingtang10]

see #1496

[aurangzaibumer]

I was testing the behavior after the latest changes, the validation error shows (if applicable) when focus is switched to next UI element. I did test on emulator (Pixel 5 API 31). Previously it used to be running like runtime (while we have focus on the same view).

Is this the final desired behavior or please correct me If I'm wrong? Thanks

cc @jingtang10 @f-odhiambo @maimoonak @shoaibmushtaq25

latestchange.mp4