-
Notifications
You must be signed in to change notification settings - Fork 66
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
Conversation
0d034dd
to
10b626f
Compare
10b626f
to
be57705
Compare
be57705
to
e3afde3
Compare
There was a problem hiding this 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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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' |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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?
e3afde3
to
dc4dbf9
Compare
dc4dbf9
to
08c3f04
Compare
There was a problem hiding this 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.
08c3f04
to
760aa9e
Compare
There was a problem hiding this 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@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? |
…#101919) (#20499) * Create migration for PoA form submissions (#101919) * Changes per Oren review
This is PR No. 1 of a stack concerning ticket #101919
Summary
Related issue(s)
Testing done
rake db:migrate
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
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?