chore: Normalize User.tms #10992
chore: Normalize User.tms #10992
Conversation
packages/client/modules/userDashboard/components/UserTasksHeader/UserTasksHeader.tsx
Outdated
Show resolved
Hide resolved
|
Views are clunky when altering the underlying table. We can achieve the same with just a custom loader. |
Dschoordsch
force-pushed
the
chore/9932/normalizeUserTms
branch
from
9035ff1 to
c44c37b
Compare
Dschoordsch
force-pushed
the
chore/9932/normalizeUserTms
branch
from
b1f681c to
3099ba3
Compare
We're selecting `User` on a low level, introducing the resolver dependency breaks types in Chronos.
| export async function down(db: Kysely<any>): Promise<void> { | ||
| await db.schema | ||
| .alterTable('User') | ||
| .addColumn('tms', sql`character varying(100)[]`, (col) => col.defaultTo('{}').notNull()) |
There was a problem hiding this comment.
varchar(100)[] means that the textual representation of the ENTIRE array must be < 100 chars, i.e. this will break if you join 10 or 12 teams.
There was a problem hiding this comment.
Isn't that the same what we currently have or am I missing something?
There was a problem hiding this comment.
🤔 THAT IS INTERESTING!
Either my understanding is wrong or we've got a problem! will investigate...
| eb | ||
| .selectFrom('TeamMember') | ||
| .select((se) => se.fn('array_agg', ['teamId']).as('tms')) | ||
| .where('TeamMember.userId', '=', eb.ref('User.id')), |
There was a problem hiding this comment.
should ignore inactive teams and teamMember objects that have been removed from the team
There was a problem hiding this comment.
Makes sense, will fix.
There was a problem hiding this comment.
That archived team really got me. It's not enough to check the TeamMember but we also need to check the Team the team member points to :(
| await db | ||
| .updateTable('User') | ||
| .set((eb) => ({ | ||
| tms: eb.fn.coalesce( |
There was a problem hiding this comment.
question-- i don't understand how this PR works 😅 we're setting the tms in a migration, but have removed all the places where we update the tms column?
There was a problem hiding this comment.
You're looking at the down migration. We're dropping tms in up, that's why the update code gets deleted.
| @@ -57,6 +59,7 @@ const removeFromOrg = async ( | |||
| dataLoader.get('users').loadNonNull(userId) | |||
| ]) | |||
| dataLoader.clearAll('organizationUsers') | |||
| const {tms} = user | |||
There was a problem hiding this comment.
running yarn pg:generate should remove tms from the schema & will make finding things like this easier, at least i hope!
There was a problem hiding this comment.
I've updated the description. Although I'm removing tms from the User table, I'm still querying it in the dataloader. I guess a different solution could also have been to add a trigger in PG to just get rid of the update logic in code. For performance reasons, especially for socket connects/disconnects I did not want to double the roundtrips to the db.
Description
Partially Fixes #9932
The
tmsarray was removed fromUserin postgres as it is redundant to the information we can obtain by queryingTeamMember. Because we often need this array to check permissions, I wanted to avoid the additional roundtrip to the db necessary to construct the array fromTeamMember. Instead thetmsarray is queried already in the dataloader.Demo
Nothing should change for the user.
Testing scenarios
Joining, leaving and archiving teams
Final checklist