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

[ARP POA submission] (#1) Create migration for PoA form submissions (#101919) #20499

Merged
merged 2 commits into from
Feb 12, 2025

Conversation

pixiitech
Copy link
Contributor

@pixiitech pixiitech commented Jan 28, 2025

This is PR No. 1 of a stack concerning ticket #101919

Summary

  • This work is behind a feature toggle (flipper): YES/NO No
  • (Summarize the changes that have been made to the platform) Added migration to record PoA form submissions to Lighthouse
  • (If bug, how to reproduce) feature
  • (What is the solution, why is this the solution?) ⭐ Lighthouse POA API Integration va.gov-team#101919
  • (Which team do you work for, does your team own the maintenance of this component?) ARF/ART
  • (If introducing a flipper, what is the success criteria being targeted?) -

Related issue(s)

Testing done

  • New code is covered by unit tests migration only
  • Describe what the old behavior was prior to the change migration wasn't there yet
  • Describe the steps required to verify your changes are working as expected. Exclusively stating 'Specs run' is NOT acceptable as appropriate testing rake db:migrate
  • If this work is behind a flipper:
    • Tests need to be written for both the flipper on and flipper off scenarios. Docs.
    • What is the testing plan for rolling out the feature?

Screenshots

Note: Optional

What areas of the site does it impact?

(Describe what parts of the site are impacted andifcode touched other areas) ARP portal

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 (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

Requested Feedback

(OPTIONAL)What should the reviewers know in addition to the above. Is there anything specific you wish the reviewer to assist with. Do you have any concerns with this PR, why?

Not Found
@va-vfs-bot va-vfs-bot temporarily deployed to art/101919/create-poa-form-submissions/main/main January 28, 2025 19:26 Inactive
@pixiitech pixiitech force-pushed the art/101919/create-poa-form-submissions branch from 0d034dd to 10b626f Compare January 29, 2025 14:15
@va-vfs-bot va-vfs-bot temporarily deployed to art/101919/create-poa-form-submissions/main/main January 29, 2025 14:27 Inactive
@pixiitech pixiitech force-pushed the art/101919/create-poa-form-submissions branch from 10b626f to be57705 Compare January 31, 2025 15:29
@va-vfs-bot va-vfs-bot temporarily deployed to art/101919/create-poa-form-submissions/main/main January 31, 2025 15:47 Inactive
@pixiitech pixiitech force-pushed the art/101919/create-poa-form-submissions branch from be57705 to e3afde3 Compare January 31, 2025 20:12
@va-vfs-bot va-vfs-bot temporarily deployed to art/101919/create-poa-form-submissions/main/main January 31, 2025 20:13 Inactive
Copy link
Contributor

@nihil2501 nihil2501 left a comment

Choose a reason for hiding this comment

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

Qs

t.string :service_id
t.jsonb :service_response_ciphertext
t.string :status, default: 'pending'
t.datetime :status_updated_at
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's decide on whether we should use both updated_at and status_updated_at? To me it seems like we'll only make updates when we see a new status update coming from the status checking process, so only one column is needed, and if the statement on what this update is is true, then maybe status_updated_at is more precise?

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 removed updated_at

t.uuid :power_of_attorney_request_id, null: false
t.string :service_id
t.jsonb :service_response_ciphertext
t.string :status, default: 'pending'
Copy link
Contributor

Choose a reason for hiding this comment

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

With this refined synchronization strategy, I believe the status values would need to be updated as well. Perhaps it is now these values and column needs to be NOT NULL?

enqueue_succeeded
enqueue_failed
succeeded
failed

class CreateArPowerOfAttorneyFormSubmissions < ActiveRecord::Migration[7.2]
def change
create_table :ar_power_of_attorney_form_submissions do |t|
t.uuid :power_of_attorney_request_id, null: false
Copy link
Contributor

Choose a reason for hiding this comment

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

We should make schema enforce the has_one which is something we've done elsewhere as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should make schema enforce the has_one which is something we've done elsewhere as well.

Would this be done using a index: { unique: true }? This would ensure only one form submission per PoA request? Will this not defeat the purpose of keeping errored records?

Copy link
Contributor

@nihil2501 nihil2501 Feb 6, 2025

Choose a reason for hiding this comment

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

Would this be done using a index: { unique: true }?

Yes, but I recall seeing platform guidance on OJ's last commit here to separate index creation (and run it "safely"). I don't know if they'll give us an exception for an empty table, I think this point has come up multiple times but I don't think I ever noticed their final point of view.

Anyway, if it is in a second migration, I don't know whether you'd get the nice inline syntax or not with a change_column statement or something like that, but if so, that would be the nice inline syntax.

This would ensure only one form submission per PoA request? Will this not defeat the purpose of keeping errored records?

Good point. Thinking back to this comment in the ticket about how we're incrementally modeling towards generic form submission attempts elsewhere in the VA codebase:

This design could maybe be thought of as a redux of possible future integration w/ these platform models:

  • saved_claims
  • form_submissions
  • form_submission_attempts
  • claim_va_notifications

Anyway, we don't have any imagined functionality or way for us as operators to initiate further attempts. A failed submission will be a terminal point of that POA request's lifecycle. Also, we'll be representing the same dumb thing to the frontend: one submission.

t.jsonb :service_response_ciphertext
t.string :status, default: 'pending'
t.datetime :status_updated_at
t.text :error_message_ciphertext
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we settled on this single error message or do we want a jsonb complying with JSON:API spec's error object? "Errors" section of ticket talks about what would populate this data.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems JSONB doesn't play well with ciphertext anyway, it's still requiring a string

Copy link
Contributor

@nihil2501 nihil2501 Feb 6, 2025

Choose a reason for hiding this comment

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

form_submissions has an encrypted jsonb column. But it does seem like it could be pointless to have this encrypted data be jsonb. I don't think it gives benefits beyond using a text column once it's deserialized into Ruby and because it's encrypted, we don't get to query into it in SQL anyway. Not totally sure on these points though.

Copy link
Contributor

Choose a reason for hiding this comment

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

If all that is true maybe service_response_ciphertext would be text also.

Copy link
Contributor

Choose a reason for hiding this comment

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

And for your broader point that going with text wouldn't stop us from saving json, I agree. The only remaining contention then would be the name error_message, but again, I think we don't need to convince ourselves that the more complex thing needs to be set in stone right now.

t.datetime :status_updated_at
t.text :error_message_ciphertext

t.timestamps
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe just created_at (see above discussion about updated_at).

def change
create_table :ar_power_of_attorney_form_submissions do |t|
t.uuid :power_of_attorney_request_id, null: false
t.string :service_id
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if there is value to the DB being set up with a constraint that service_id should be unique among our form submission records?

@nihil2501 nihil2501 changed the title Create migration for PoA form submissions (#101919) [ARP POA submission] Create migration for PoA form submissions (#101919) Feb 3, 2025
@pixiitech pixiitech force-pushed the art/101919/create-poa-form-submissions branch from e3afde3 to dc4dbf9 Compare February 4, 2025 21:57
@va-vfs-bot va-vfs-bot temporarily deployed to art/101919/create-poa-form-submissions/main/main February 4, 2025 21:58 Inactive
@pixiitech pixiitech requested a review from nihil2501 February 5, 2025 14:58
@pixiitech pixiitech force-pushed the art/101919/create-poa-form-submissions branch from dc4dbf9 to 08c3f04 Compare February 5, 2025 20:20
@va-vfs-bot va-vfs-bot temporarily deployed to art/101919/create-poa-form-submissions/main/main February 5, 2025 20:21 Inactive
nihil2501
nihil2501 previously approved these changes Feb 6, 2025
Copy link
Contributor

@nihil2501 nihil2501 left a comment

Choose a reason for hiding this comment

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

LGTM after whatever changes you wanted to do based on my latest set of replies.

Seems like we're going to simplify to an error message string and not bother trying to capture structured error information for now. We had some idea of storing LH API's structured failure info for use as operators, but we can smush it into semi-structured text if we really want and reevaluate another day.

@pixiitech pixiitech changed the title [ARP POA submission] Create migration for PoA form submissions (#101919) [ARP POA submission] (#1) Create migration for PoA form submissions (#101919) Feb 6, 2025
@pixiitech pixiitech force-pushed the art/101919/create-poa-form-submissions branch from 08c3f04 to 760aa9e Compare February 6, 2025 14:43
@va-vfs-bot va-vfs-bot temporarily deployed to art/101919/create-poa-form-submissions/main/main February 6, 2025 14:45 Inactive
Copy link
Contributor

@nihil2501 nihil2501 left a comment

Choose a reason for hiding this comment

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

LGTM

Your choice on whether to add unique index now, later, or indefinitely-not.

@pixiitech pixiitech marked this pull request as ready for review February 6, 2025 20:43
@pixiitech pixiitech requested review from a team as code owners February 6, 2025 20:43
Copy link
Contributor

@nihil2501 nihil2501 left a comment

Choose a reason for hiding this comment

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

LGTM

@nihil2501
Copy link
Contributor

@pixiitech This is passing all checks now. LGTM, but I don't quite remember if there is anything needed since other migrations have gone in more recently.

Just guessing, but I think Rails still sees that the migration needs to run since it won't be present in the schema versions table. And then there is only potentially significance to the migration timestamp itself when there is a dependency order between 2 migrations, which wouldn't be the case here.

Does that seem right?

@pixiitech pixiitech merged commit 84cedbb into master Feb 12, 2025
59 checks passed
@pixiitech pixiitech deleted the art/101919/create-poa-form-submissions branch February 12, 2025 15:15
holdenhinkle pushed a commit that referenced this pull request Feb 12, 2025
…#101919) (#20499)

* Create migration for PoA form submissions (#101919)

* Changes per Oren review
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants