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
54 stars 41 forks source link

QR to HTML Population #3259

Closed FikriMilano closed 3 months ago

FikriMilano commented 4 months ago

IMPORTANT: Where possible all PRs must be linked to a Github issue

Fixes #3257

Engineer Checklist

Code Reviewer Checklist

codecov[bot] commented 4 months ago

Codecov Report

Attention: Patch coverage is 88.37209% with 10 lines in your changes missing coverage. Please review.

Project coverage is 28.4%. Comparing base (ac82739) to head (269af5a). Report is 82 commits behind head on main.

Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/opensrp/fhircore/pull/3259/graphs/tree.svg?width=650&height=150&src=pr&token=IJUTHZUGGH&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=opensrp)](https://app.codecov.io/gh/opensrp/fhircore/pull/3259?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=opensrp) ```diff @@ Coverage Diff @@ ## main #3259 +/- ## ========================================= - Coverage 29.6% 28.4% -1.3% - Complexity 658 718 +60 ========================================= Files 239 267 +28 Lines 11204 12578 +1374 Branches 1948 2212 +264 ========================================= + Hits 3323 3576 +253 - Misses 7447 8532 +1085 - Partials 434 470 +36 ``` | [Flag](https://app.codecov.io/gh/opensrp/fhircore/pull/3259/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=opensrp) | Coverage Δ | | |---|---|---| | [engine](https://app.codecov.io/gh/opensrp/fhircore/pull/3259/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=opensrp) | `65.1% <88.3%> (-1.1%)` | :arrow_down: | | [geowidget](https://app.codecov.io/gh/opensrp/fhircore/pull/3259/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=opensrp) | `18.7% <ø> (-28.5%)` | :arrow_down: | | [quest](https://app.codecov.io/gh/opensrp/fhircore/pull/3259/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=opensrp) | `5.2% <ø> (-0.3%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=opensrp#carryforward-flags-in-the-pull-request-comment) to find out more. | [Files](https://app.codecov.io/gh/opensrp/fhircore/pull/3259?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=opensrp) | Coverage Δ | | |---|---|---| | [...hircore/engine/util/extension/DateTimeExtension.kt](https://app.codecov.io/gh/opensrp/fhircore/pull/3259?src=pr&el=tree&filepath=android%2Fengine%2Fsrc%2Fmain%2Fjava%2Forg%2Fsmartregister%2Ffhircore%2Fengine%2Futil%2Fextension%2FDateTimeExtension.kt&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=opensrp#diff-YW5kcm9pZC9lbmdpbmUvc3JjL21haW4vamF2YS9vcmcvc21hcnRyZWdpc3Rlci9maGlyY29yZS9lbmdpbmUvdXRpbC9leHRlbnNpb24vRGF0ZVRpbWVFeHRlbnNpb24ua3Q=) | `90.3% <100.0%> (+0.3%)` | :arrow_up: | | [...e/util/extension/QuestionnaireResponseExtension.kt](https://app.codecov.io/gh/opensrp/fhircore/pull/3259?src=pr&el=tree&filepath=android%2Fengine%2Fsrc%2Fmain%2Fjava%2Forg%2Fsmartregister%2Ffhircore%2Fengine%2Futil%2Fextension%2FQuestionnaireResponseExtension.kt&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=opensrp#diff-YW5kcm9pZC9lbmdpbmUvc3JjL21haW4vamF2YS9vcmcvc21hcnRyZWdpc3Rlci9maGlyY29yZS9lbmdpbmUvdXRpbC9leHRlbnNpb24vUXVlc3Rpb25uYWlyZVJlc3BvbnNlRXh0ZW5zaW9uLmt0) | `100.0% <100.0%> (ø)` | | | [...hircore/engine/util/extension/ResourceExtension.kt](https://app.codecov.io/gh/opensrp/fhircore/pull/3259?src=pr&el=tree&filepath=android%2Fengine%2Fsrc%2Fmain%2Fjava%2Forg%2Fsmartregister%2Ffhircore%2Fengine%2Futil%2Fextension%2FResourceExtension.kt&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=opensrp#diff-YW5kcm9pZC9lbmdpbmUvc3JjL21haW4vamF2YS9vcmcvc21hcnRyZWdpc3Rlci9maGlyY29yZS9lbmdpbmUvdXRpbC9leHRlbnNpb24vUmVzb3VyY2VFeHRlbnNpb24ua3Q=) | `65.9% <100.0%> (-0.1%)` | :arrow_down: | | [...smartregister/fhircore/engine/pdf/HtmlPopulator.kt](https://app.codecov.io/gh/opensrp/fhircore/pull/3259?src=pr&el=tree&filepath=android%2Fengine%2Fsrc%2Fmain%2Fjava%2Forg%2Fsmartregister%2Ffhircore%2Fengine%2Fpdf%2FHtmlPopulator.kt&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=opensrp#diff-YW5kcm9pZC9lbmdpbmUvc3JjL21haW4vamF2YS9vcmcvc21hcnRyZWdpc3Rlci9maGlyY29yZS9lbmdpbmUvcGRmL0h0bWxQb3B1bGF0b3Iua3Q=) | `86.6% <86.6%> (ø)` | | ... and [10 files with indirect coverage changes](https://app.codecov.io/gh/opensrp/fhircore/pull/3259/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=opensrp)
ndegwamartin commented 4 months ago

This implementation could be refactored for some optimization e.g. use one while loop with if else statements for the various conditions during parsing or using regex for the various parse/replace logic.

The performance gains are probably negligible for the size of HTML we are parsing so will go ahead and approve this. We can keep a look out and perform any refactors accordingly incase latency is noticable.

ndegwamartin commented 4 months ago

cc @FikriMilano

FikriMilano commented 4 months ago

@pld @ndegwamartin Thanks for the review, I'll do some R&D before finalizing the improvement

FikriMilano commented 4 months ago

@pld @ndegwamartin there's a higher priority in building the HTML templates and working on pending engage tickets.

So, will get back to this PR later.

FikriMilano commented 3 months ago

@pld here's a use case where we should not increment i

  1. Given: <p>@is-not-empty('link')Text@is-not-empty('link')</p>
  2. The first '@' will be detected (since it's a tag), and it's index is 3
  3. If answer is not empty, both tags will be replaced by 'Text'
  4. Since the index is not incremented, it stays 3 in the next iteration, but the value of the index has changed from '@' to 'T'
  5. Index 3 with 'T' as it's value is detected
  6. The flow goes to the ELSE (since it's not a tag), only then the index got incremented, from 3 to 4, with value respectively 'T' to 'e'
  7. On the next iteration, index 4 will go to the ELSE, and the process continues

So, this is why we should not increment i after processing a tag.