Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add support for Enekto draw widget in forms #8904

Merged
merged 15 commits into from
Jun 14, 2024

Conversation

ChinHairSaintClair
Copy link
Contributor

@ChinHairSaintClair ChinHairSaintClair commented Feb 21, 2024

Description

Closes #8308
Closes #8072

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.

@garethbowen
Copy link
Member

@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?

@jkuester
Copy link
Contributor

jkuester commented May 9, 2024

👍 Yes, I believe that draw/signature is backed by the same libraries (maybe even the same Enekto widget... 🤔 ).

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?

@garethbowen
Copy link
Member

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: #8865

@jkuester
Copy link
Contributor

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

@jkuester
Copy link
Contributor

@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!

@garethbowen
Copy link
Member

That size change should not hold up this widget.

@jkuester
Copy link
Contributor

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

# Conflicts:
#	webapp/src/ts/services/enketo.service.ts
@jkuester
Copy link
Contributor

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

@ChinHairSaintClair
Copy link
Contributor Author

ChinHairSaintClair commented May 30, 2024

@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

@jkuester
Copy link
Contributor

@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. 🤔

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

@mrjones-plip
Copy link
Contributor

mrjones-plip commented May 30, 2024

@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

# Conflicts:
#	webapp/tests/karma/ts/services/enketo.service.spec.ts
@jkuester
Copy link
Contributor

jkuester commented Jun 8, 2024

Okay, I am going to try and clarify/recap what is going on in this PR! 😅 The main goal is to address #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 #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! 🙏 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. 😬

@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
Copy link
Contributor

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

Copy link
Contributor

@lorerod lorerod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jkuester Thank you for sharing the details of the testing approach. It sounds well thought out!
Regarding the draw-widget.wdio.spec.ts test, asserting the image size is a clever simplification. It ensures something substantial is saved and avoids overcomplicating the tests.
Keeping tests focused will make them easier to manage and less prone to breaking with future changes. Regarding the similarity between submit-photo-upload-form.wdio-spec.js and file-upload.wdio-spec.js, I agree with the justification behind keeping them separate to keep the tests modular and focused on their specific workflows and avoid adding unnecessary complexity to the e2e tests.
I also manually tested the enketo draw widget. It works! Here is a screenshot of the report, and extract from the saved doc:
image

"_attachments": {
    "user-file-drawing-15_5_28.png": {
      "content_type": "image/png",
      "revpos": 1,
      "digest": "md5-X49lAzhPmcGO+8DwomyxSA==",
      "length": 20381,
      "stub": true
    }
  }

Copy link
Contributor

@Benmuiruri Benmuiruri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I was able to test and see how we extend the enketo file manager and providing the widgets in Webapp.

@ChinHairSaintClair
Copy link
Contributor Author

@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
Copy link
Contributor Author

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

@jkuester
Copy link
Contributor

@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 jkuester changed the title feat: draw widget feat: add support for Enekto draw widget in forms Jun 14, 2024
@jkuester jkuester merged commit 1afebb7 into medic:master Jun 14, 2024
17 of 19 checks passed
@ChinHairSaintClair ChinHairSaintClair deleted the draw_widget branch June 18, 2024 10:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for Signature Widget in forms Error when saving form with repeated upload inputs
7 participants