Migration for PR #5660 #5664
Migration for PR #5660 #5664
Conversation
|
Preview URL 🚀 : https://blurts-server-pr-5664-mgjlpikfea-uk.a.run.app |
flozia
force-pushed
the
mntor-4065-migrations
branch
2 times, most recently
from
516c52c to
855444a
Compare
…rofiles_table.js into its own PR
flozia
force-pushed
the
mntor-4065-migrations
branch
from
bbbb69d to
0340c6c
Compare
src/db/migrations/20250218094907_alter_and_add_profile_details_to_onerep_profiles_table.js
Outdated
Show resolved
Hide resolved
| export function up(knex) { | ||
| return knex.schema.alterTable("onerep_profiles", async (table) => { | ||
| table | ||
| .specificType("first_names", "character varying(255)[]") |
There was a problem hiding this comment.
With backend/DB design not being my speciality, I don't want to have a strong opinion here, but I suppose the following are reasons why it's OK to use array types and JSON rather than the more cumbersome foreign tables:
- We don't care about DBMS portability,
- we won't be selecting rows based on the contents of these columns,
- we'll rarely write to these columns, and
- when we do write to them, we'll often update all elements in one go.
Writing this down more for myself, but happy to learn if anyone has corrections or additions.
There was a problem hiding this comment.
I’ve considered adding a new table, but that might be too much overhead. We are already using jsonb in a couple of places. That means:
- we don’t have DBMS portability either way
- I don’t anticipate that we need to select rows based on these columns
- writing to those columns happens only if users update their profile info
- we only update the elements in those columns all at once
src/db/migrations/20250218094907_alter_and_add_profile_details_to_onerep_profiles_table.js
Outdated
Show resolved
Hide resolved
| table.dropColumns( | ||
| "first_names", | ||
| "last_names", | ||
| "middle_names", | ||
| "phone_numbers", | ||
| "addresses", | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Rolling back the migration doesn't actually drop these columns for me...
There was a problem hiding this comment.
They do if I remove the knex.raw above. Possibly that should not be part of the alterTable function? (And that function then needn't be async?)
| table.dropColumns( | ||
| "first_names", | ||
| "last_names", | ||
| "middle_names", | ||
| "phone_numbers", | ||
| "addresses", | ||
| ); | ||
| }); |
There was a problem hiding this comment.
I wish we didn't need so much knex.raw here, but I agree that we're not aiming to be RDBMS-agnostic at all, and arrays are a very useful feature of Postgres.
The code looks fine but my main concern is how this is tested, we should have a plan for how to test this on stage.
I'm a little worried about the rollback down function, we should make sure to test that thoroughly. It would be far easier to roll back than to restore a database backup, but I really don't want to have to use it.
src/db/migrations/20250218094907_alter_and_add_profile_details_to_onerep_profiles_table.js
Outdated
Show resolved
Hide resolved
|
Closed in favor of PR #5696 to workaround the stuck Deploy Preview Action as suggested by @mansaj. |
|
Cleanup completed - database 'blurts-server-pr-5664' destroyed, cloud run service 'blurts-server-pr-5664' destroyed |
References:
PR: #5660
Jira: MNTOR-4065
Description
Migration file for PR #5660 that is adding new optional columns to
onerep_profiles.Screenshot (if applicable)
Not applicable.
How to test
npm run db:migrate: The tableonerep_profilesshould contain the applied changes.npm run db:rollback: The tableonerep_profilesshould have its initial state.