Fixed Showing no validation errors when form opens

[aurangzaibumer]

Fixes
#1001 #opensrp/fhircore#873

Description
Removed validation from being triggered when user opens the form and enters no data.

Type
Choose one: ( Bug fix)

Screenshots (if applicable)
https://user-images.githubusercontent.com/35099184/152525877-d7d52896-3063-4925-903e-159a34d86a32.mp4

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 reference app(s) to verify my change fixes the issue and/or does not break the reference app(s)

[joiskash]

Can you please update the PR template. You can refer to other PRs to know how to do this. eg: #1106

[aurangzaibumer]

Can you please update the PR template. You can refer to other PRs to know how to do this. eg: #1106

thanks, just updated the template.

[codecov]

Codecov Report

Merging #1102 (8387ff6) into master (7a19c97) will decrease coverage by 0.11%.
The diff coverage is 72.22%.

@@             Coverage Diff              @@
##             master    #1102      +/-   ##
============================================
- Coverage     83.72%   83.60%   -0.12%     
+ Complexity      678      677       -1     
============================================
  Files           147      147              
  Lines         10670    10676       +6     
  Branches        862      866       +4     
============================================
- Hits           8933     8926       -7     
  Misses         1287     1287              
- Partials        450      463      +13     

Impacted Files	Coverage Δ
...iews/QuestionnaireItemEditTextViewHolderFactory.kt	42.55% <44.44%> (+2.55%)	⬆️
...acapture/validation/RequiredConstraintValidator.kt	100.00% <100.00%> (ø)
...estionnaireItemEditTextStringViewHolderDelegate.kt	85.71% <100.00%> (-3.18%)	⬇️
...apture/views/QuestionnaireItemViewHolderFactory.kt	100.00% <100.00%> (ø)
...e/views/QuestionnaireItemGroupViewHolderFactory.kt	52.17% <0.00%> (-8.70%)	⬇️
...iews/QuestionnaireItemDropDownViewHolderFactory.kt	43.58% <0.00%> (-5.13%)	⬇️
...views/QuestionnaireItemDisplayViewHolderFactory.kt	55.00% <0.00%> (-5.00%)	⬇️
...uestionnaireItemDateTimePickerViewHolderFactory.kt	19.46% <0.00%> (-3.54%)	⬇️
...tionnaireItemBooleanTypePickerViewHolderFactory.kt	53.12% <0.00%> (-3.13%)	⬇️
...QuestionnaireItemCheckBoxGroupViewHolderFactory.kt	67.56% <0.00%> (-2.71%)	⬇️
... and 2 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7a19c97...8387ff6. Read the comment docs.

[jingtang10]

Please take a look at this section of the wiki: https://github.com/google/android-fhir/wiki/Structured-Data-Capture-Library#provide-configuration

[jingtang10]

discussed offline with @aurangzaibumer: @ellykits and @aurangzaibumer please consider using the DataCaptureConfg class. Application developers should be able to set the config using the link I posted above.

If that is indeed a problem, then please let us know so we can make sure the APIs are easy to use.

Just a final note, this way of providing library config is consistent with other android libraries such as work manager.

[ellykits]

@jingtang10 This is well noted. There was no problem using the DataCaptureConfg class. However, one potential issue we may encounter in FHIRCore (currently using Hilt for Dependency Injection) is in the tests that would require the application to be cast into DataCaptureConfig.Provider instance. Hilt provides HiltTestApplication that is recommended to be used in the instrumentation tests instead of a custom test application class.

Note that it is possible to provide a custom application for Hilt tests however for best practices it is not recommended to avoid accidental leaks of state used in the application class, that is documented here Hilt custom test application.

As an alternative approach to implementing the DataCaptureConfig.Provider interface via the Application class we can consider providing the DataCaptureConfig through a singleton initialized in the onCreate method of the Application. Implementing a custom Hilt test application for our tests would also still address our concern.

[jingtang10]

@jingtang10 This is well noted. There was no problem using the DataCaptureConfg class. However, one potential issue we may encounter in FHIRCore (currently using Hilt for Dependency Injection) is in the tests that would require the application to be cast into DataCaptureConfig.Provider instance. Hilt provides HiltTestApplication that is recommended to be used in the instrumentation tests instead of a custom test application class.

Note that it is possible to provide a custom application for Hilt tests however for best practices it is not recommended to avoid accidental leaks of state used in the application class, that is documented here Hilt custom test application.

As an alternative approach to implementing the DataCaptureConfig.Provider interface via the Application class we can consider providing the DataCaptureConfig through a singleton initialized in the onCreate method of the Application. Implementing a custom Hilt test application for our tests would also still address our concern.

thanks @ellykits - i don't really understand the reasoning behind Hilt's recommendation besides "it's easy to get it wrong". @yigit @stevenckngaa @kevinmost fyi. let me do some research and get back to you on this one.

[kevinmost]

As a best practice, avoid using @CustomTestApplication and instead use HiltTestApplication in your tests. In general, having your Activity, Fragment, etc. be independent of the parent they are contained in makes it easier to compose and reuse it in the future.

Sounds like the main concern is that it introduces a stronger coupling between your Hilt Application and the component (Fragment, Activity, etc)?

[jingtang10]

i chatted with someone working on hilt and they said: It is just a 'recommendation', we know it might not be possible for all situations to only use the HiltTestApplication which is why there is also an API for custom applications.

so i think it's ok to use the custom application in testing as long as we're aware of the potential (avoidable) issues.

[ellykits]

Cool thanks.

…

On Wed, Feb 16, 2022, 12:30 Jing Tang ***@***.***> wrote: i chatted with someone working on hilt and they said: It is just a 'recommendation', we know it might not be possible for all situations to only use the HiltTestApplication which is why there is also an API for custom applications. — Reply to this email directly, view it on GitHub <#1102 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AGM667BL4B4OUKECQOB4CDLU3NVD5ANCNFSM5NLRF7UA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[jingtang10]

Can you add a test case for RequiredConstraintValidator.kt?

Actually if you do this, you can probably remove the new test cases you added in the boolean and single line widgets' tests.

[aurangzaibumer]

Can you add a test case for RequiredConstraintValidator.kt?

Actually if you do this, you can probably remove the new test cases you added in the boolean and single line widgets' tests.

I've added few test cases for the RequiredConstraintValidator.kt class as per the business. Can you please review. thanks
@jingtang10

[aurangzaibumer]

Comments are resolved. Can you please review @jingtang10 . Thanks
cc @f-odhiambo

[1371523503]

什么

…

---Original--- From: ***@***.***&gt; Date: Thu, Apr 7, 2022 01:10 AM To: ***@***.***&gt;; Cc: ***@***.***&gt;; Subject: Re: [google/android-fhir] Fixed Showing no validation errors whenform opens (PR #1102) Comments are resolved. Can you please review @jingtang10 . Thanks cc @f-odhiambo &mdash; Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you are subscribed to this thread.Message ID: ***@***.***&gt;

[aurangzaibumer]

Unable to update with master branch @jingtang10
@f-odhiambo

[aurangzaibumer]

Unable to update with master branch @jingtang10 @f-odhiambo

fixed, can update now.

[jingtang10]

I think the test failures reveal a problem with this code change.

we're relying on the difference between a questionnaire item without answer, and a questionnaire with an answer without value to distinguish questions that have been answered but the answers have been cleared and questions that have not been answered.

but this is not strong enough.

problem 1) hapi actually doesn't make this distinction. The hasAnswer api returns only answers with non empty values, so it actually makes it quite tricky for us to call the right api (have to avoid using hasAnswer and only use answer.size) 2) in the case of multiple answers, we actually have no way of saving an answer without value... so taht's not gonna work for questions with repeated answers.

i think we probably should introduce a "dirty" field for each question in the view model.

[jingtang10]

Two possible solutions (pending investigation):

1. instead of always creating a QuestionnaireResponseItem in the QuestionnaireItemViewItem, we can leave it as null if the question hasn't been touched by the user at all. If this field is null (meaning there's no answer at all), we don't validate. But if it's not null, we will validate. And if the non-null QuestionnaireResponseItem has no answer, that means the user has tried to answer it but left it empty/blank. In this case if the question is required, we should show validation error.
2. Add a dirty field to indicate if the question has been touched by the user

[jingtang10]

@aurangzaibumer @ellykits

[aurangzaibumer]

New PR Link #1309