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

Rescue CombinePDF::ParsingError on 8879 signature pages #4521

Merged
merged 2 commits into from
May 21, 2024

Conversation

jenny-heath
Copy link
Contributor

@jenny-heath jenny-heath commented Apr 29, 2024

Link to pivotal/JIRA issue

What was done?

Investigation summary: I think the reason we get this error is that sometimes Hub users upload wonky PDFs as unsigned 8879 document types and our code that signs this document barfs and results in the client seeing an error page. I recreated this by finding the clients who got this error (for once the Sentry page had an ID of a record) and trying to sign one of their unsigned 8879 documents locally.

In order to avoid showing the error page I've done a very minimal fix to catch the error where we catch other errors (e.g. when the client has already signed). This isn't necessarily that helpful to the client but at least the banner says to contact support. There could be a more robust solution in future but we already have a feature that asks the Hub user to confirm that what they're uploading is an unsigned 8879 PDF, and this has only happened to 5 people.

How to test?

I added one test per signing service—basically copies of the existing tests around catching errors.

Acceptance testing proposal:

  • To see existing behavior on demo: as a hub user upload a malformed PDF as an unsigned 8879 and log in as the client to sign it. See that you get an error page. To get a malformed PDF, try this one from one of the clients who got this error.
  • To see the new behavior on the heroku app: do the same thing. Maybe also try uploading a legit unsigned 8879 and see that signing it still works.

Screenshots (for visual change)

before (obviously this looks different on non-dev environments):
image

after (the second banner):
image

Copy link

Heroku app: https://gyr-review-app-4521-1fe422b86a10.herokuapp.com/
View logs: heroku logs --app gyr-review-app-4521 (optionally add --tail)

@jenny-heath jenny-heath marked this pull request as ready for review April 29, 2024 18:10
Copy link
Contributor

@rickreyhsig rickreyhsig left a comment

Choose a reason for hiding this comment

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

This seems to be aligned with the resolution discussed in boazsegev/combine_pdf#28.

@tofarr tofarr merged commit 473febc into main May 21, 2024
7 checks passed
@tofarr tofarr deleted the rescue-pdf-parsing-error-#187083156 branch May 21, 2024 18:09
@tofarr
Copy link
Contributor

tofarr commented May 21, 2024

I added my review to this and merged since Jenny is on leave

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.

None yet

3 participants