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

VA forms: Add form 21P-0847 #12960

Merged
merged 0 commits into from
Jun 12, 2023
Merged

VA forms: Add form 21P-0847 #12960

merged 0 commits into from
Jun 12, 2023

Conversation

gmrabian
Copy link
Contributor

Add form to va forms_api capabilities

Summary

  • Add form id to controller-accepted values
  • Add generated model and mapping files, with updated values to match frontend
  • Add custom stamp method for signature field
  • Add test payloads with minimum and maximum fields
  • Enable tests for form
  • Add PDF for form generation

Related issue(s)

Testing done

  • Tested locally with tests and with frontend

What areas of the site does it impact?

21P-0847 (non prod)

Acceptance criteria

  • I fixed|updated|added unit tests and integration tests for each feature (if applicable).
  • No error nor warning in the console.
  • Events are being sent to the appropriate logging solution
  • No sensitive information (i.e. PII/credentials/internal URLs/etc.) is captured in logging, hardcoded, or specs
  • Feature/bug has a monitor built into Datadog or Grafana (if applicable)

Copy link
Contributor

@jeremy6d jeremy6d left a comment

Choose a reason for hiding this comment

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

See comments, should be small change, and the pipeline looks messed up anyway re: that one failing code check


multistamp(generated_form_path, stamp_path)
rescue
Rails.logger.error "Failed to generate stamped file: #{e.message}"
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is going to work unless you make it rescue => e. And I'd really recommend capturing specific exceptions rather than any exception, but I won't block you on that.

Copy link
Contributor

Choose a reason for hiding this comment

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

and maybe test exception handling if you're going to leave it so open ended?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for calling that out! I refactored the stampers to cut down on that reused logic, added the => e and added tests for errors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I still left the error generic because there are a few failure points, and which one it is isn't important for us right now. I will keep that suggestion in mind.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's totally your call

@gmrabian gmrabian merged commit cae9321 into master Jun 12, 2023
@gmrabian gmrabian deleted the forms_api/add-form-21p-0847 branch June 12, 2023 18:51
ryan-mcneil pushed a commit that referenced this pull request Dec 11, 2023
* initially generated files for 21p-0847

* add form mappings and tests

* rename files

* one more name change

* refactor multistamp methods and add tests
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.

4 participants