4: [ART] Make POA request and form adjustments migration

[nihil2501]

Description

This PR adds a migration to modify the Power of Attorney forms table structure by:

1. Adding BIDX (binary index) columns for city, state, and zip code
2. Adding ciphertext columns for secure storage of address data
3. Adding claimant_type column to power_of_attorney_requests table

Details

• Removes existing index on city_bidx_state_bidx_zipcode_bidx
• Removes individual zipcode index
• Safely removes legacy columns (city_bidx, state_bidx, zipcode_bidx)
• Adds new columns for encrypted data storage:
  • claimant_city_ciphertext/bidx
  • claimant_state_code_ciphertext/bidx
  • claimant_zip_code_ciphertext/bidx
• Creates a new concurrent index on BIDX columns
• Adds claimant_type column to requests table

[ryan-mcneil]

Is it absolutely necessary to add columns to both models at the same time? Generally I would avoid this unless I HAVE to

[amprokop]

@ojbucao can comment but I don't see why it's necessary. Is your suggestion to split this up into two separate migrations, just so running & reverting seem cleaner?

[ryan-mcneil]

Actually, we request that indexes are modified in separate transactions migrations. I can already hear a few of you questioning this :) but we've seen things fail, and when it does, it requires a lot of work on our side. So please break this down into separate PRs for each of the index updates, and the two models (unless it's absolutely necessary the two models be updated at the same time)

[amprokop]

Different transactions or different PRs?

All of these tables should have 0 records in all deployed environments as this isn't yet a production feature, or even one that's been tested in staging, does that make a difference to you?

[amprokop]

(I agree with the guidance on having index updates in separate transactions, just not sure of the case for making them separate PRs, they'll run separately anyway if split up into diff migrations no)

[ojbucao]

I can split these up into separate migrations.

[ryan-mcneil]

guys - I didn't make the rules. Fighting me is only going to get your work less attention.

[ryan-mcneil]

I had intended to say migrations, which we generally would split into PRs, and is why I stated it that way. I suppose separate migrations in the same PR is ok.

[amprokop]

I wasn't trying to fight you on the rules! Correct me if I'm wrong and I will apologize profusely but you said two separate things, one of which was transparently reasonable and one of which was harder to understand, and I was trying to distinguish:)

[amprokop]

Not sure if this is necessary, since these tables should have 0 records in all fields, do we know why it's in here or just out of habit/pattern-matching?

[ojbucao]

probably just habit. actually no, db:migrate throws an error without it.

[ryan-mcneil]

@ojbucao I was looking over the PR when you force pushed - I'm keeping an eye out for your team approval, then we'll be good to go

[ojbucao]

@ojbucao I was looking over the PR when you force pushed - I'm keeping an eye out for your team approval, then we'll be good to go

Thank you!

[ryan-mcneil]

I want to note here that I brought this up in the CoP this morning, and that while it's not always necessary, we generally recommend following the same patterns for removing indexes as we do for adding (regarding transactions/concurrency). Considering there aren't any records in the table, we'll let it go here.

[raudabaugh]

I understand the reasoning here, but I’d lean towards sticking with our pattern of using algorithm: :concurrently even if it seems unnecessary in this case. It ensures consistency and might reduce risk if the migration is ever redone in a different context. Would it make sense to make that change as a precaution?

[ryan-mcneil]

Lastly I'll mention that in the future, we'd generally recommend/request separate PRs for the various types of migrations, just because they're easier to revert if, for example, the 3rd out of 5 migrations fail for whatever reason, and can't be easily rolled back because it's outside of a transaction. But in this case, as we're dealing with models without any data (in production... I didn't look at staging), and they're related operations on the same table, we're ok with letting them go in together this time. Thanks for breaking them up, though

[LindseySaari]

Giving this a 👍 but please keep an eye on things when you merge (dev and staging). Due to the use of disable_ddl_transaction, the index removal is not wrapped in a transaction, which means that if something goes wrong during the migration process, the index could be left in a partially removed state, requiring manual intervention to resolve