Skip to content

chore: Normalize User.tms #10992

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

Merged
merged 3 commits into from
Jul 1, 2025
Merged

chore: Normalize User.tms #10992

merged 3 commits into from
Jul 1, 2025

Conversation

Dschoordsch
Copy link
Contributor

@Dschoordsch Dschoordsch commented Mar 12, 2025

Description

Fixes #9932
We query User.tms quite often to check permissions, so it's useful to be denormalized. To avoid update errors, I added triggers for Team and TeamMember operations which update the array.
There is also some inconsistent data in the database, I've added a migration to clean that up as well.

Demo

Nothing should change for the user.

Testing scenarios

Joining, leaving and archiving teams

Final checklist

  • I checked the code review guidelines
  • I have added Metrics Representative as reviewer(s) if my PR invovles metrics/data/analytics related changes
  • I have performed a self-review of my code, the same way I'd do it for any other team member
  • I have tested all cases I listed in the testing scenarios and I haven't found any issues or regressions
  • Whenever I took a non-obvious choice I added a comment explaining why I did it this way
  • I added the label Skip Maintainer Review Indicating the PR only requires reviewer review and can be merged right after it's approved if the PR introduces only minor changes, does not contain any architectural changes or does not introduce any new patterns and I think one review is sufficient'
  • PR title is human readable and could be used in changelog

Copy link
Contributor Author

@Dschoordsch Dschoordsch left a comment

Choose a reason for hiding this comment

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

I'm not sure solving this in code is the right call for the tms field as it's used in the connection logic which should be kept lean.
I'm thinking about making a UserWithTeams view for this use case.

@Dschoordsch
Copy link
Contributor Author

Views are clunky when altering the underlying table. We can achieve the same with just a custom loader.

@Dschoordsch Dschoordsch force-pushed the chore/9932/normalizeUserTms branch 2 times, most recently from b1f681c to 3099ba3 Compare March 18, 2025 10:07
@Dschoordsch Dschoordsch marked this pull request as ready for review March 18, 2025 15:41
@github-actions github-actions bot requested a review from tianrunhe March 18, 2025 15:42
@Dschoordsch Dschoordsch marked this pull request as draft March 19, 2025 10:30
@tianrunhe tianrunhe removed their request for review March 19, 2025 17:48
User.tms must reflect User<-TeamMember->Team relation considering
Team.isArchived and TeamMember.isNotRemoved. We query it always with the
User and thus it's sensible to keep it in the User. To keep it
consistent, we're updating it with triggers now.
@Dschoordsch Dschoordsch force-pushed the chore/9932/normalizeUserTms branch from 898ee9d to 2854b82 Compare June 25, 2025 13:55
@github-actions github-actions bot added size/m and removed size/l labels Jun 25, 2025
@github-actions github-actions bot added size/m and removed size/m labels Jun 27, 2025
@github-actions github-actions bot added size/m and removed size/m labels Jun 27, 2025
@Dschoordsch Dschoordsch marked this pull request as ready for review June 27, 2025 12:55
.onConflict((oc) => oc.columns(['id']).doUpdateSet({isNotRemoved: true}))
.execute()

const res = await db
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This takes around 20s on staging and affects 250 users.

@Dschoordsch Dschoordsch requested a review from mattkrick June 27, 2025 12:58
@Dschoordsch Dschoordsch merged commit a8934c5 into master Jul 1, 2025
11 checks passed
@Dschoordsch Dschoordsch deleted the chore/9932/normalizeUserTms branch July 1, 2025 04:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

chore: normalize data in PG across tables
2 participants