cht-core icon indicating copy to clipboard operation
cht-core copied to clipboard

feat: draw widget

Open ChinHairSaintClair opened this issue 1 year ago • 7 comments

Description

Enables the draw widget, which allows CHT to capture free hand drawings through touch or mouse pointer. image

License

The software is provided under AGPL-3.0. Contributions to this project are accepted under the same license.

ChinHairSaintClair avatar Feb 21 '24 19:02 ChinHairSaintClair

@jkuester I'd love your input here. This feels related to #8308 . Maybe we need to think about how to optionally include specific widgets and/or bundle them separately?

garethbowen avatar May 09 '24 08:05 garethbowen

:+1: Yes, I believe that draw/signature is backed by the same libraries (maybe even the same Enekto widget... :thinking: ).

I will try to carve out some time in the next week to think about optional widgets. On a related note, @garethbowen I see that the bundle size limits were removed with Grunt. Were these replaced with something else or do we have anything right now that is checking the size of the bundle?

jkuester avatar May 09 '24 16:05 jkuester

Were these replaced with something else or do we have anything right now that is checking the size of the bundle?

No they weren't replaced. In reality we weren't blocking on that anyway. It's not a practical solution because every commit gets you closer and closer to the limit until finally one unlucky soul hit it. So every time we breached it we just arbitrarily bumped it until we disabled it anyway.

IMO this is a better solution: https://github.com/medic/cht-core/issues/8865

garethbowen avatar May 09 '24 17:05 garethbowen

Have not forgotten about this. Planning to do some investigating today or tomorrow! Will follow up out here with any findings.

jkuester avatar May 16 '24 18:05 jkuester

@garethbowen I did a deep dive into the options for modularlizing/lazy-loading widgets and included my findings in the issue. The TLDR with regards to this PR, though is:

  • I could not find any low-hanging-fruit that would make it easy to pick and choose widgets. There are a number of options, but they would all take a decent amount of effort to pull off.
  • The cost of just merging this PR, as is, will be +9.28 KB to the Gzipped bundle size (total webapp Gzipped size going from 2.64 MB to 2.65 MB)

Let me know how you want to proceed!

jkuester avatar May 17 '24 19:05 jkuester

That size change should not hold up this widget.

garethbowen avatar May 20 '24 08:05 garethbowen

:+1: Sounds good! My plan this week, then, is to do a proper review of these code changes and either suggest or just push some unit tests and some integration/cht-form tests.

@lorerod just wanted to get you engaged at the start here in case you had any thoughts/concerns going into this. I am thinking we will want at least 1 happy-path e2e test to make sure we cover saving the drawing image as an attachment in Couch, but besides that, I will try to cover other test cases in the cht-form integration tests...

jkuester avatar May 20 '24 15:05 jkuester

Bad news. The draw widget implementation in our current Enketo version (^7.2.0) apparently has serious issues and it was reverted in was reverted in 8.1.0. I believe we will have to block this PR until after we upgrade our Enketo version (but @garethbowen let me know if you agree!). Read on for more details:

While testing this code, I uncovered an issue where, if the signature is not on the last page (but on one of the earlier pages) and the signature widget is still visible on the page when the "Next" button is hit, the image will not be saved for that signature. This bug is not a result of the code changes in this PR, but I thought it might be some kind of integration issue with how we are manipulating eneketo-core. However, after extensive debugging over the last two days, I began to wonder if this was a bug in the actual Enekto widget code.

This suspicion led me to a review of the latest enketo-core source code where I realized our unlucky timing here... In enketo-core 7.2.0, they implemented a significant shift in the draw widget implementation. Several issues were identified with this functionality (though I did not find anything logged that reflected the exact behavior I was observing). Eventually, the decision was made to completely revert the earlier changes and this was released in enketo-core 8.1.0. I did try testing the draw widget from enketo-core 7.1.0 and could not reproduce the bug that I had observed before. My current thinking is that we should hold off on adding this functionality until after we uplift our enketo version in https://github.com/medic/cht-core/issues/8755.


For the record, the bug that I was tracking in the implementation of the draw widget from 7.2.0 to 8.0.0 was in the ResizableSignaturePad.js code. Basically, there are two observers. The resizeObserver listens for changes to the size of the canvas entities and then re-draws things accordingly. The intersectionObserver listens for changes in the visibility of the canvas entities (e.g. them appearing on a new page or just scrolling in/out of view) and adds/removes the canvases from the resizeObserver's tracking list as they become visible/hidden. The bugged behavior happens when clicking the "Next" button on a page where the draw widget is still visible (and so the resizeObserver is still tracking the canvas). When the transition to the next page occurs, the current page gets hidden and this causes the offsetWidth/offsetHeight of the canvas to be set to 0. I believe what follows is technically a race condition, but I never saw the outcome change. Hiding the current page triggers both the intersectionObserver and the resizeObserver, but the resizeObserver always wins and executes first. This is a problem because the intersectionObserver is supposed to prevent the resizeObserver from ever getting executed for a canvas that is not visible. Calling the resizeObserver with the offsetWidth/offsetHeight of 0 triggers an unintended flow that ultimately results in the canvas height getting set to 0. This, in turn, means that when you call toDataURL() on the canvas, if will return "data:," instead of the actual base64 canvas data. (And this is why the bugged flow ends with no image attachment getting saved.)

One potential fix I had identified was to update the ResizableSignaturePad code to add a check in the resizeObserver so that it would confirm pad.isVisible was true before proceeding to call pad.redraw().

jkuester avatar May 29 '24 21:05 jkuester

@jkuester, thank you for the detailed findings while investigating this!

Am I correct in assuming that CHT version 4.3.x also uses Enketo 7.2.0, or is it still on 7.1.0? The reason I ask is that we're currently using this drawing widget in our deployment, and it seems to be functioning as intended.

It would definitely impact our decision/haste in upgrading to newer CHT versions. Please see this form, where our drawing widget is on the 2nd of 8 pages. Below, the signature can be seen showing correctly in the report view: image The attachment can also viewed by using the below URL (after a report has been captured): http://localhost:5984/medic/0b5ec2c2-cded-4375-b09b-e231d1a9002f/user-file/csharp-householdconsentandquestionnaire/hhsignatures/consent_hhrespondentsignature

ChinHairSaintClair avatar May 30 '24 11:05 ChinHairSaintClair

@ChinHairSaintClair good question! We upgraded to Enekto ^7.2.0 in CHT 4.6.0. So, CHT 4.3.x would not include the version of Enekto with the bugged Draw widget.

That being said, your question has made me reconsider our options here! If we look at the Enketo PR that reverted the Draw widget changes for Enketo 8.1.0, we see that the actual code changes were completely isolated to the two files in the widget/draw directory and none of the actual core/util code in enketo-core was affected. This means that it should be trivial for us to copy the working draw widget code from Enekto 8.1.0 into cht-core as a "custom widget" that we use along side our current Enekto ^7.2.0. Then, in the future, when we do complete the upgrade to Enekto 8.1.0+ it will be completely passive to remove the custom draw widget code and switch to depending on the Draw widget provided by Enketo. :thinking:

@garethbowen let me know if you are open to pursuing that approach (or should we just double-down on efforts to complete the Enketo uplift?).

jkuester avatar May 30 '24 13:05 jkuester

@jkuester - per our private chat in slack, let's go ahead with this approach:

trivial for us to copy the working draw widget code from Enekto 8.1.0 into cht-core as a "custom widget"

We can be sure to undo any patchwork we do when we get around to the Enketo uplift

mrjones-plip avatar May 30 '24 15:05 mrjones-plip

Okay, I am going to try and clarify/recap what is going on in this PR! :sweat_smile: The main goal is to address https://github.com/medic/cht-core/issues/8308 so we can support collecting signatures in forms. (The enketo draw widget also supports collecting general drawings as well as annotations where you upload an image and then draw on it. We get these for "free" along with the signature support.) Unfortunately, as noted above, the draw widget for our current version of enekto-core is bugged, so we have copied the working version of the widget into our codebase (and will remove it when we upgrade to the latest version of enekto-core).

The draw widget handles all the client-facing/display logic for us, but the cht-core code still needs to handle (in enketo.service.ts) taking the signature images saved in the form and storing them as attachments on the Couch document for the report. Since we were going to need to add a bunch of additional logic here to support attaching the drawing images, we decided instead to take this opportunity to switch to using the Enketo file-manager (or really, our custom extension of the file-manager) to load the files from the form. The downside of this is that it means we had to change the naming pattern that we were previously using for Couch doc attachments (which, in turn, meant we had to update the code in a couple places to account for the new naming scheme). The upside of this is that it fixes https://github.com/medic/cht-core/issues/8072 and we can now support attaching files within repeats!


@ChinHairSaintClair I think this is finally ready to go! When you get the chance, can you pull down the latest version of this branch and try it out to make sure everything still works for you?

@Benmuiruri I am hoping you might have some time next week to do a code review of this! :pray: I am not sure how much experience you have had with the Enekto stuff so far, but if you get totally confused by looking at this, let me know and we can jump on a call to discuss! (Our "integration" with the enekto-core library is very involved (read: "tightly coupled"). These changes are not making it much better, but they are not making it too much worse either. :grimacing:

@lorerod the testing here got a bit interesting and I would very much appreciate your feedback on it! In the cht-form integration tests, I added draw-widget.wdio.spec.ts which tests a form that has all three types of draw widget appearances (draw, signature, and annotate). I did manage to get wdio to "draw" a shape on the canvases, but I did not go so far as to actually assert the image contents. Instead, I went with asserting the size of the image as a simplification that would at least allow us to confirm that something non-trivial was saved for those questions. Then, I added file-upload.wdio-spec.js with a small form for testing adding image file uploads from within a repeat group.

I thought about adding either of these workflows to the existing enketo_widgets tests either in the cht-form integration tests or the default e2e tests. However, my current thinking is that we are probably better off, from a long-term maintainability perspective, if we stick with smaller more targeted forms/tests instead of cramming everything in a big catch-all form. Interested on your thoughts here! Also, you will note that the submit-photo-upload-form.wdio-spec.js file is pretty similar to file-upload.wdio-spec.js. I thought about just adding a repeat to that test, but I did not really want the extra complexity in the e2e tests (instead of in the cht-form integration tests). Then I thought about just migrating those tests to the file-upload.wdio-spec.js, but ultimately thought it best to leave them in the e2e tests because the submit-photo-upload-form.wdio-spec.js is currently covering these workflows (that could not be adequately testing in the integration tests):

  • Ensure attached image is displayed on the reports page after the form is submitted
  • Ensure that a form with image attachment can be edited and a new image attached (or not)

jkuester avatar Jun 08 '24 02:06 jkuester

FYI:

  • The failing k3d test suits are a known issue caused by this PR originating in an external branch. I tried pushing the same code into a branch in medic/cht-core and the tests pass just fine. For the purposes of this PR we can disregard those test failures.
  • I have reached out for additional translations and will include them all in this PR once I get the remaining ones
  • @Benmuiruri for manually testing the draw widgets, you can use the enketo_widgets form from this support-scripts PR

jkuester avatar Jun 12 '24 14:06 jkuester

@jkuester Seems to be working fine on my side too 😃! Thank you for capturing the investigation and dev journey in such detail. image

"_attachments": {
    "user-file-signature-12_15_38.png": {
      "content_type": "image/png",
      "revpos": 1,
      "digest": "md5-8FXoQ/m15sQB93wznbFYrQ==",
      "length": 7645,
      "stub": true
    }
  }

ChinHairSaintClair avatar Jun 14 '24 10:06 ChinHairSaintClair

Noticed that the mechanism of viewing the the attachment via a URL no longer seems to work. URL structure: <domain>/<database>/<record_id>/user-file/<form_name>/<group_name>/<property>

On old instance: http://localhost:5984/medic/f925d4aa-b4d9-4edf-8c07-cf87e29f8afd/user-file/csharp-householdconsentandquestionnaire/hhsignatures/consent_hhrespondentsignature image image

New instance: http://localhost:5984/medic/09625306-6a8a-4af0-906d-508a10cdc6af/user-file/consent/death_details/signature image image

ChinHairSaintClair avatar Jun 14 '24 12:06 ChinHairSaintClair

@ChinHairSaintClair thanks for testing this out! I think your behavior difference with the URLs is actually to be expected since we changed the naming convention for the attachments. Instead of /user-file/consent/death_details/signature, the new path to the attachment would just be user-file-signature-12_15_38.png (the name of the attachment being associated with the value stored for the death_details/signature field).

jkuester avatar Jun 14 '24 18:06 jkuester