search

opensrp / fhircore

FHIR Core / OpenSRP 2 is a Kotlin application for delivering offline-capable, mobile-first healthcare project implementations from local community to national and international scale using FHIR and WHO Smart Guidelines on Android.
https://smartregister.org
Apache License 2.0
53 stars 41 forks source link

Support for validating resources before saving to local db #2853

Closed LZRS closed 1 month ago

LZRS commented 10 months ago

Describe the feature request. Add support for validating FHIR resources before getting saved to local DB to ensure that no invalid resources are saved. This would also help ensure that when sync to server, requests do not fail because of resource being invalid

Additional context If available add any other context or screenshots about the feature here.

Acceptance criteria A check list of all things to verify once the implementation by the engineer is complete

Area path A list of ordered steps in the app on usage of the feature to support anyone testing it e.g. Code reviewer, QA e.g.

  1. Login to the app
  2. Open Navigation bar
  3. Click on Children register
  4. Click on Child profile
  5. Click on Edit profile from menu

Implementation plan (For Engineers) The plan for implementing the solution e.g. via a description or a check list for the various ordered tasks that will need to be completed. i.e. Describe how you intend to solve the problem

dubdabasoduba commented 10 months ago

@pld I think this would be a good addition to the FHIR Core app too. We can make it configurable with an opt out capability.

pld commented 10 months ago

kinda agree! but.. what happens if they are invalid? once someones trying to store a resource we wouldn't want to not persist it, I guess in theory we could redo the extraction after the config has been updated, like you could mark the QR as extraction pending or something.

But I feel like the more impactful place for effort would be on validating structuremaps to ensure they are only producing valid resources as outputs

ndegwamartin commented 10 months ago

Suppose we had a variant of this that only makes this type of validation check mandatory for Debug mode. That way during development it is easier for engineers to catch such early on. This can be in place regardless of the conclusion we make for this ticket.

dubdabasoduba commented 10 months ago

@ndegwamartin I agree this is more for the development stage and not necessary for use in production. The assumption here is that all SMs and CQL would generate correctly formatted resources by the time an app goes live

dubdabasoduba commented 8 months ago

@LZRS how far are we with this? cc @ndegwamartin

LZRS commented 7 months ago

@LZRS how far are we with this? cc @ndegwamartin

With this PR the validation of resources for debug, before saving to db, was added but ran into issues to do with dependency incompatibilities for the caffeine library that was causing the app to crash whenever a validation happened

Replacing the caffeine library caching with guava, using the latest engine sdk artifacts, fixes the issue but it means work on validation essentially depends on the work/merge of #2825

The validation checks that the FHIR resources conform to the defined structure, references match the expected types (that is, without checking if they refer to non-existing resources) and that code systems used are known (although, for now, marked as just warnings). Failed validations are logged to logcat, and questionnaire submission fails to succeed until all validations pass (while in debug)

Pending enhancement, would be to allow skipping to submission anyways, as some errors of validations could be treated with leniency

pld commented 7 months ago

@LZRS can we show the user the failed validation directly on the screen in a way they can copy it? the users of debug will be testers on our and other teams so their workflow will be

  1. submit form
  2. get error
  3. copy error data into new issue

I think your suggestion is brings up a very good point. If the tester hits a non-critical error, we don't want them to be unable to continue testing until that non-critical error is resolved. How about we show the error in a very big clear way so the tester knows to create an issue, but we continue with the submission so that they can continue testing the workflow.

LZRS commented 7 months ago

Yeah, makes sense. I will be looking into maybe showing the errors in a dialog just before continuing with submission. The dialog would allow the users to copy the errors and also probably have an extra button to either allow to share the errors (maybe to slack, github or email) or do background submission of the errors to sentry, email or even github

ndegwamartin commented 1 month ago

I think the target audience for this is mainly the developers. QA team will never get to see this since it is recommended they test the production release. Adding 3rd party integrations for on UI reporting/error forwarding can be for some version 2 that targets production builds. For now, log cat error feedback should be sufficient for v1 cc @dubdabasoduba @pld

pld commented 1 month ago

OK sounds good