department-of-veterans-affairs / va.gov-cms

Editor-centered management for Veteran-centered content.
https://prod.cms.va.gov
GNU General Public License v2.0
96 stars 69 forks source link

Find Forms: Add download modal on form detail pages #18855

Open randimays opened 1 month ago

randimays commented 1 month ago

Description

On form detail pages, the download link should open the same modal that is present on the Find Forms search results page. If a PDF is opened in the browser, the user will not be able to save it potentially causing a lot of lost time and work and a frustrating experience. We can't change the user's default settings, but we can guide them to use a PDF reader to open the document (which is the purpose of the modal).

Page examples:

Form detail pages download link

Screenshot 2024-08-06 at 11.51.02 AM.png

Find Forms search results page

Screenshot 2024-08-06 at 11.52.29 AM.png

User story

AS A Veteran, their caregiver, family member or survivor I WANT to be prompted to use, and/or download Adobe Acrobat to fill our my PDF forms SO THAT I don't open a PDF in my browser, fill it our, then be frustrated when I lose my information.

Engineering notes / background

Analytics considerations

Quality / testing notes

Acceptance criteria

FranECross commented 1 month ago

@Agile6MSkinner Just an FYI that I moved this to the top of the Ready pipeline for Randi to take care of in Sprint 10.

randimays commented 4 weeks ago

Update: I have added back the modal, but I am blocked in a few ways by the find forms API. In short: I cannot test this locally or in the review instances. I have two PRs open for this work; one for content-build and one for vets-website.

The content-build PR is a very simple change to update the links on form detail pages to buttons since they will now be triggering modals. I have tried to verify this in the review instance in the PR but the forms API is not working; the call fails completely with no status code. Platform support ticket here.

The vets-website PR has the bulk of the changes. Review instances have been broken for a couple of days so the RI for this PR is not working at all.

Running the code locally, I am also running into API issues. I have been alternating between pointing to dev-api and staging-api for the Find Forms API calls, and regardless the calls are still going to localhost (which means calls fail / PDFs throw 404s).

Strangely, when I test a form detail page locally that has related forms (example: https://www.va.gov/find-forms/about-form-10-10ez/), 2 of the 3 calls to retrieve form details on that page are hitting localhost, and the one for the Spanish language form is hitting staging. This is a mystery because the API calls are made exactly the same way for all 3 forms. Even though the Spanish language form is hitting staging, the results from the forms DB for that call (10-10EZS) are specific to English language forms, so the flow still fails and shows the alert banner. Slack thread describing this.

randimays commented 3 weeks ago

EOD Update:

Daniel helped to identify the issue with the Spanish form above: the staging environment's form data is different from production. The prod /v0/forms API is able to retrieve the form with the name 10-10EZS, but staging doesn't have it. We have a Platform ticket open attempting to resolve it: https://dsva.slack.com/archives/CBU0KDSB1/p1723837400852169.

Locally, I'm still running into issues with the forms API hitting localhost instead of staging, and failing to retrieve forms, so I can't get the modal on the form detail page to open--it fails and shows the alert banner instead.

Both PRs (for VW and CB) are linked to this ticket. I was at the point where I think only testing is needed (and resolving any issues that come from testing), but I'm not sure how to get around the API blockers.

jilladams commented 3 weeks ago

Slack thread here with Platform and LH forms team that hopefully can unstick this: https://dsva.slack.com/archives/CUB5X5MGF/p1724349694751589. Highlights:

Lower env syncs w/ upstream lower env where possible, but it seems like catch-22... lowers are required to use mocks.......but then the mocks get out of sync with the real production data.....i guess re syncing will help, but only for a limited amount of time......... Typically Lighthouse API consumers use sandbox and prod only. sandbox is a mocked environment but should have data parity w/ prod for this API both sandbox and prod sync data (nightly) from the prod CMS. Would it be possible for you to work on sandbox? It's an immediately available option that is a URL change as opposed to a manual sync that may only last for a day until the next lower-env sync.

So to avoid the mismatch of prod data in lower envs issue, if we can point to the Sandbox for testing, that would give us prod data. We'll verify with Randi after her PTO.

cc. @dsasser

randimays commented 1 week ago

Update: I was able to get some help from Daniel, Platform and the Lighthouse team to find a form detail page with multiple form links that we could test locally. But we are still limited with our local testing because:

  1. form detail pages check not only for the existence of the form in the /v0/forms API call, but they also validate that the given (prod) URL is valid before popping the modal, and: a. forms don't exist in the local environment, so this will always 404 and block the modal b. we can't run the HTTP request against the prod URL locally because it throws a CORS error, and again blocks the modal

TLDR: I still had to make manual code changes locally to get the modal to show up.

The PR has a detailed description with testing steps, screenshots and video (Loom) walkthroughs. But due to the limitations above and the fact that these changes are both content-build and vets-website, we can't use review instances or Tugboat to test. PRs are in review now and we'll be strategic about merging both PRs for plenty of testing time in staging.

Another update: We decided to add feature flags to both the VW and CB code to reduce risk of shipping the changes. Once all this work ships successfully, I'll create a ticket to remove the feature flags.

jilladams commented 1 week ago

my spidey senses suddenly wondered if we need to revise any understandings about how the code executes / alarms after this change. Notes from a convo I had with Chris a long long time ago are here: https://github.com/department-of-veterans-affairs/va.gov-team/blob/master/products/find-a-va-form/engineering/monitoring.md#causes-for-each-of-the-errors-in-the-fe-code-the-errors-will-occur-in-this-order. I'm guessing no, but t would be helpful to confirm.

randimays commented 1 week ago

@jilladams Thanks for that docs callout; I made some updates to the docs here: https://github.com/department-of-veterans-affairs/va.gov-team/commit/71821639823ead5549f01ae318813ea9d966fbdb. The way the APIs are called did not change but the explanation needed a bit of finessing.

randimays commented 1 day ago

Update here: I added a feature flag to the content-build changes only because it renders either a button or a link on the form detail page. Locally, when it rendered the link, it downloaded the form regardless of what the vets-website widget was doing. When it rendered the button, it showed the modal.

I got the vets-website PR approved today and merged it in, optimistically thinking the content-build PR would follow -- but I did not notice the content-build PR had an unrelated failing CI step. While I was triaging that, the vets-website changes went to staging, so I checked them. I discovered that the links on the form detail pages were not working, even though they were link tags with hrefs. My thinking (in only putting a feature flag on the content-build changes) was that the vets-website changes would not interfere with what is already in content-build in prod. That was incorrect, so I'm reverting the vets-website changes and adding the VW feature flag I originally intended to add.