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
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions db/schema.rb

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
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.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?

t.text :service_response_ciphertext
t.string :status, null: false
t.text :encrypted_kms_key
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.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 :created_at, null: false
end
end
end