Skip to content

LG-14815: rename document_capture_sessions column socure_docv_token to socure_docv_transaction_token#11360

Merged
amirbey merged 4 commits intomainfrom
amirbey/LG-14815-rename-docv-token
Oct 18, 2024
Merged

LG-14815: rename document_capture_sessions column socure_docv_token to socure_docv_transaction_token#11360
amirbey merged 4 commits intomainfrom
amirbey/LG-14815-rename-docv-token

Conversation

@amirbey
Copy link
Contributor

@amirbey amirbey commented Oct 17, 2024

🎫 Ticket

Link to the relevant ticket:
LG-14815

🛠 Summary of changes

Rename column from socure_docv_token to socure_docv_transaction_token in document_capture_sessions table

Copy link
Contributor

Choose a reason for hiding this comment

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

Acknowledging this is draft and may change, just making sure to call out that these migrations would need to happen over multiple deploys, including code changes to reference both possible column names when in 50/50 state.

https://handbook.login.gov/articles/manage-50-50-state.html#database-changes

Copy link
Contributor Author

@amirbey amirbey Oct 17, 2024

Choose a reason for hiding this comment

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

@aduth - while the column socure_docv_token already exists in the database, its use it sits behind a feature flag and has not yet been used in dev, staging nor prod. also the column is only written to and not read from. i believe this should be able to go in 1 deploy, correct?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, in that case, could we collapse the two migrations to a single migration which renames the column? Or at least drops and creates the new column in one go.

https://api.rubyonrails.org/v7.1/classes/ActiveRecord/ConnectionAdapters/SchemaStatements.html#method-i-rename_column

@amirbey amirbey requested a review from aduth October 17, 2024 15:11
Copy link
Contributor

Choose a reason for hiding this comment

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

I wish StrongMigrations' documentation included more details than just "will cause errors", but based on their "safer approach" guidelines, I do think we'd be safe if we're not currently using this column.

Copy link
Contributor

Choose a reason for hiding this comment

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

There's also a rename_column_safely helper from StrongMigrations, which preserves the expressivity while doing it the way they recommend. Based on previous comment, don't know that it's strictly necessary.

@amirbey amirbey requested review from a team and solipet and removed request for a team October 17, 2024 16:40
@amirbey amirbey marked this pull request as ready for review October 17, 2024 16:40
@amirbey amirbey requested a review from aduth October 17, 2024 16:40
@amirbey amirbey changed the title rename docv_token to docv_transaction_token LG-14815: rename docv_token to docv_transaction_token Oct 17, 2024
@amirbey amirbey changed the title LG-14815: rename docv_token to docv_transaction_token LG-14815: rename document_capture_sessions column socure_docv_token to socure_docv_transaction_token Oct 17, 2024
Copy link
Contributor

@aduth aduth left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@solipet solipet left a comment

Choose a reason for hiding this comment

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

LGTM!

@amirbey amirbey force-pushed the amirbey/LG-14815-rename-docv-token branch from 18d8321 to ee9917f Compare October 18, 2024 17:22
@amirbey amirbey merged commit d220668 into main Oct 18, 2024
@amirbey amirbey deleted the amirbey/LG-14815-rename-docv-token branch October 18, 2024 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants