DBEX: Form526Submission submit_endpoint data migration #16693

freeheeling · 2024-05-08T23:05:40Z

Rake task to migrate historic Form526Submission data, setting all submit_endpoint values to evss, signifying the service API used for past and current claim submissions.

Also included in this PR:

In the near future, with the migration from EVSS to Lighthouse services, the submission of form 526 will be transitioning to the Benefits Claims API, at which time a submission's submit_endpoint will initially be set as claims_api.
The eminent addition of a toxic exposure section to form 526 also necessitates routing any form including this section to the the claims_api endpoint.
Whether a Veteran has been presented with the option to provide toxic exposure details will be indicated, when submitting a claim, with the passing of a includes_toxic_exposure query param.
A submission's submit_endpoint is updated to benefits_intake_api, which receives all backup submissions of form 526 that fail primary delivery to EVSS.

Summary

This work is behind a feature toggle (flipper): ~~YES~~/NO
- not directly, but it will be leveraged in conjunction with the disability_526_toxic_exposure Flipper
Follow-on to DBEX: add submit_endpoint to form526_submissions #16689
Defines enum submit_endpoint attribute, and tests behavior
Updates newly added submit_endpoint attribute for all pre-existing Form526Submission records to have an enum value of evss
Data migration files and CODEOWNERS entry will be removed following confirmation of deployment to production per VA Platform guidance
Conditionally sets the submit_endpoint, based on the inclusion/value of a includes_toxic_exposure query param, as well as updating the endpoint's value if submission is processed via a backup process
Responsible team: Disability Benefits Experience Team 2 (dBeX Carbs 🥖)

Related issue(s)

Testing done

Ran rake task locally against a set of Form526Submission records
Created new and supplemented existing unit, request and background job tests
What is the testing plan for rolling out the feature?
- Verify via the console that the submit_endpoint value of all Form526Submission records has been updated to evss after running the rake task

What areas of the site does it impact?

Form 526EZ

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
Documentation has been updated (link to documentation)
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)
If app impacted requires authentication, did you login to a local build and verify all authenticated routes work as expected
I added a screenshot of the developed feature

va-vsp-bot · 2024-05-08T23:06:07Z

Error: A file (or its parent directories) does not have a CODEOWNERS entry. Please update the .github/CODEOWNERS file and add the entry for the Offending file: lib/data_migrations/form526_submission_submit_endpoint_update.rb

va-vsp-bot · 2024-05-09T18:48:03Z

Error: A file (or its parent directories) does not have a CODEOWNERS entry. Please update the .github/CODEOWNERS file and add the entry for the Offending file: lib/data_migrations/form526_submission_submit_endpoint_update.rb

…int-data-migration

NB28VT

Main blocker is the question about setting variables in the tests and skipping model validations in the backfill script kickoff, and some style nits.

NB28VT · 2024-05-14T16:35:14Z

app/controllers/v0/disability_compensation_forms_controller.rb

@@ -118,7 +118,8 @@ def create_submission(saved_claim)
        user_account: @current_user.user_account,
        saved_claim_id: saved_claim.id,
        auth_headers_json: auth_headers.to_json,
-        form_json: saved_claim.to_submission_data(@current_user)
+        form_json: saved_claim.to_submission_data(@current_user),
+        submit_endpoint: includes_toxic_exposure? ? :claims_api : :evss


Nit: looks like the linter didn't mind but the ternary operator here is a little hard to read with the symbols, may be consider doing this in a method with if statements

Utilized strings in place of symbols for improved readability. (see d2a1c09)

Good call - one sanity check just confirming it doesn't matter downstream if you change the type of this value?

I believe the most common method for setting the value of an enum attribute is by using the symbolic key, but using the string representation of the symbolic key is also valid.

NB28VT · 2024-05-14T16:39:44Z

lib/data_migrations/form526_submission_submit_endpoint_update.rb

+      Form526Submission.where(submit_endpoint: nil).in_batches do |batch|
+        # rubocop:disable Rails/SkipsModelValidations
+        batch.update_all(submit_endpoint: :evss)
+        # rubocop:enable Rails/SkipsModelValidations


Hmm is this an existing precedent for data migrations? I feel like skipping validations is quite dangerous

Following Platform's guidance here as well as optimizing performance when bulk updating.

Hmm I didn't see anything in there about skipping validations am I missing something? Is there a reason why you needed to skip them? At the very least if there is a good reason it would be good to have it as a comment and in the git history even if the file gets removed

what does the Validation look like if we added it? I can understand both points here.

we know what we are doing. we created a new column, and are updating all the old claims with the correct value.

lib/sidekiq/form526_backup_submission_process/submit.rb

rakelib/form526_submission_submit_endpoint_update.rake

spec/models/form526_submission_spec.rb

NB28VT · 2024-05-14T16:49:52Z

spec/requests/disability_compensation_form_request_spec.rb

+          post("/v0/disability_compensation_form/submit_all_claim#{query_param}", params: all_claims_form, headers:)
+          expect(response).to have_http_status(:ok)
+          expect(response).to match_response_schema('submit_disability_form')
+          submission = Form526Submission.last


Using first and last to test a side effect of an action always makes me nervous because it's not deterministic. Is there any way around this you can think of?

If the /v0/disability_compensation_form/submit_all_claim endpoint is designed to create a single Form526Submission record, then using Form526Submission.last to retrieve that record for testing should be reliable. But I did add to each test an expectation that there is only a single Form526Submission record. (see d2a1c09)

Yeah that's one way to get around it which is probably fine for now.

Outside of this example: my concern with using first and last in general is there could be side effects elsewhere in the code creating records you might not know about or other data created by a before block in the tests or anything like that. Or data that gets seeded for running every test. And then maybe the endpoint doesn't work as expected and because there is a record that already existed that would get returned from Form526Submission.last instead. It's probably unlikely but something to consider when writing tests. What I'm basically saying is assuming the latest record in a table was one you created without any other identifier could lead to problems if that makes sense.

spec/requests/disability_compensation_form_request_spec.rb

tblackwe · 2024-05-15T19:16:04Z

lib/data_migrations/form526_submission_submit_endpoint_update.rb

+      Form526Submission.where(submit_endpoint: nil).in_batches do |batch|
+        # rubocop:disable Rails/SkipsModelValidations
+        batch.update_all(submit_endpoint: :evss)
+        # rubocop:enable Rails/SkipsModelValidations


what does the Validation look like if we added it? I can understand both points here.

tblackwe · 2024-05-15T19:16:49Z

lib/data_migrations/form526_submission_submit_endpoint_update.rb

+      Form526Submission.where(submit_endpoint: nil).in_batches do |batch|
+        # rubocop:disable Rails/SkipsModelValidations
+        batch.update_all(submit_endpoint: :evss)
+        # rubocop:enable Rails/SkipsModelValidations


we know what we are doing. we created a new column, and are updating all the old claims with the correct value.

…int-data-migration

freeheeling · 2024-05-16T16:47:23Z

@NB28VT – regarding your change request:

Main blocker is the question about setting variables in the tests and skipping model validations in the backfill script kickoff, and some style nits.

All raised issues and suggestions have been addressed in d2a1c09. Comments corresponding to each correction have been updated to include a link to the commit.

NB28VT

Just one sanity check on the data type changing which I think is fine but worth a quick look, otherwise LGTM

Not sure where we landed re: skipping the validations, but if that's a no no I assume the platform will call it out in their review?

freeheeling added 2 commits May 8, 2024 16:36

Form526Submission submit_endpoint data migration

521dbd1

Define enum submit_endpoint attribute

435cfa1

va-vfs-bot temporarily deployed to dbex/81830-form526-submission-submit-endpoint-data-migration/main/main May 9, 2024 00:10 Inactive

Update submit_endpoint enum values

e20911d

freeheeling added 2 commits May 9, 2024 12:56

Add CODEOWNERS migrations file entry

3eef0e1

Add backend-review-group CODEOWNER to entry

f1099dc

va-vfs-bot temporarily deployed to dbex/81830-form526-submission-submit-endpoint-data-migration/main/main May 9, 2024 19:27 Inactive

Modify rake task

6c17ea2

va-vfs-bot temporarily deployed to dbex/81830-form526-submission-submit-endpoint-data-migration/main/main May 9, 2024 23:00 Inactive

Set submit_endpoint based on query param value

945ac78

va-vfs-bot temporarily deployed to dbex/81830-form526-submission-submit-endpoint-data-migration/main/main May 10, 2024 19:44 Inactive

Update submit_endpoint in backup process

e84077e

va-vfs-bot temporarily deployed to dbex/81830-form526-submission-submit-endpoint-data-migration/main/main May 10, 2024 20:58 Inactive

Remove output and update batch method

cbb753c

va-vfs-bot temporarily deployed to dbex/81830-form526-submission-submit-endpoint-data-migration/main/main May 10, 2024 22:24 Inactive

freeheeling added disability-experience To manage benefits disability claims experience. disability-compensation Label used for Pull Requests that impact Disability Compensation claims (526EZ) labels May 10, 2024

freeheeling mentioned this pull request May 10, 2024

Support two versions of 526; one without TE and one with TE fields department-of-veterans-affairs/va.gov-team#81830

Open

Make method name descriptive

e3abe39

va-vfs-bot temporarily deployed to dbex/81830-form526-submission-submit-endpoint-data-migration/main/main May 13, 2024 21:39 Inactive

freeheeling added 8 commits May 13, 2024 17:28

Form526Submission submit_endpoint data migration

42a4079

Define enum submit_endpoint attribute

34a4f06

Update submit_endpoint enum values

15762b7

Add CODEOWNERS migrations file entry

e6f9ebd

Add backend-review-group CODEOWNER to entry

e069b2b

Modify rake task

44e7339

Set submit_endpoint based on query param value

da5c6b0

Update submit_endpoint in backup process

0ecc314

freeheeling added 3 commits May 13, 2024 17:28

Remove output and update batch method

a5c6f58

Make method name descriptive

eb80b42

Merge branch 'master' into dbex/81830-form526-submission-submit-endpo…

53adf44

…int-data-migration

va-vfs-bot had a problem deploying to dbex/81830-form526-submission-submit-endpoint-data-migration/main/main May 14, 2024 00:57 Error

NB28VT requested changes May 14, 2024

View reviewed changes

freeheeling added 4 commits May 15, 2024 11:34

Refactor and revise tests

d2a1c09

Resolve conflicts

d9ca1ad

Correct resolved conflicts

6d05ae3

Remove duplicated tests

ed47a8d

va-vfs-bot temporarily deployed to dbex/81830-form526-submission-submit-endpoint-data-migration/main/main May 15, 2024 18:31 Inactive

tblackwe approved these changes May 15, 2024

View reviewed changes

Merge branch 'master' into dbex/81830-form526-submission-submit-endpo…

0cca147

…int-data-migration

va-vfs-bot deployed to dbex/81830-form526-submission-submit-endpoint-data-migration/main/main May 15, 2024 20:02 View deployment

tblackwe approved these changes May 16, 2024

View reviewed changes

NB28VT approved these changes May 16, 2024

View reviewed changes

freeheeling marked this pull request as ready for review May 16, 2024 17:53

freeheeling requested review from a team as code owners May 16, 2024 17:53

rjohnson2011 approved these changes May 17, 2024

View reviewed changes

freeheeling merged commit b2418c8 into master May 17, 2024
19 checks passed

freeheeling deleted the dbex/81830-form526-submission-submit-endpoint-data-migration branch May 17, 2024 17:14

tblackwe mentioned this pull request May 17, 2024

Implement PDF Generation for Lighthouse Submissions department-of-veterans-affairs/va.gov-team#83430

Closed

6 tasks

This was referenced May 20, 2024

DBEX: assign toxic exposure submit endpoint & metadata #16802

Merged

DBEX: explicitly load Form526Submission submit_endpoint data migration module #16823

Merged

freeheeling mentioned this pull request May 28, 2024

DBEX: add includeToxicExposure indicator to formData #16899

Draft

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DBEX: Form526Submission submit_endpoint data migration #16693

DBEX: Form526Submission submit_endpoint data migration #16693

freeheeling commented May 8, 2024 •

edited

va-vsp-bot commented May 8, 2024

va-vsp-bot commented May 9, 2024

NB28VT left a comment

NB28VT May 14, 2024

freeheeling May 15, 2024 •

edited

NB28VT May 16, 2024

freeheeling May 16, 2024

NB28VT May 14, 2024

freeheeling May 14, 2024 •

edited

NB28VT May 15, 2024

tblackwe May 15, 2024

tblackwe May 15, 2024

NB28VT May 14, 2024

freeheeling May 15, 2024 •

edited

NB28VT May 15, 2024

tblackwe May 15, 2024

tblackwe May 15, 2024

freeheeling commented May 16, 2024

NB28VT left a comment

DBEX: Form526Submission submit_endpoint data migration #16693

DBEX: Form526Submission submit_endpoint data migration #16693

Conversation

freeheeling commented May 8, 2024 • edited

Summary

Related issue(s)

Testing done

What areas of the site does it impact?

Acceptance criteria

va-vsp-bot commented May 8, 2024

va-vsp-bot commented May 9, 2024

NB28VT left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

freeheeling May 15, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

freeheeling May 14, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

freeheeling May 15, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

freeheeling commented May 16, 2024

NB28VT left a comment

Choose a reason for hiding this comment

freeheeling commented May 8, 2024 •

edited

freeheeling May 15, 2024 •

edited

freeheeling May 14, 2024 •

edited

freeheeling May 15, 2024 •

edited