chore(rethinkdb): OrganizationUser: Phase 3 #9965
Conversation
Signed-off-by: Matt Krick <[email protected]>
Signed-off-by: Matt Krick <[email protected]>
Signed-off-by: Matt Krick <[email protected]>
Signed-off-by: Matt Krick <[email protected]>
Signed-off-by: Matt Krick <[email protected]>
Signed-off-by: Matt Krick <[email protected]>
Signed-off-by: Matt Krick <[email protected]>
Signed-off-by: Matt Krick <[email protected]>
Signed-off-by: Matt Krick <[email protected]>
Signed-off-by: Matt Krick <[email protected]>
Signed-off-by: Matt Krick <[email protected]>
Signed-off-by: Matt Krick <[email protected]>
Signed-off-by: Matt Krick <[email protected]>
Signed-off-by: Matt Krick <[email protected]>
Signed-off-by: Matt Krick <[email protected]>
Signed-off-by: Matt Krick <[email protected]>
Signed-off-by: Matt Krick <[email protected]>
Signed-off-by: Matt Krick <[email protected]>
Signed-off-by: Matt Krick <[email protected]>
Signed-off-by: Matt Krick <[email protected]>
…n-phase3 Signed-off-by: Matt Krick <[email protected]>
Signed-off-by: Matt Krick <[email protected]>
Signed-off-by: Matt Krick <[email protected]>
Signed-off-by: Matt Krick <[email protected]>
Signed-off-by: Matt Krick <[email protected]>
Signed-off-by: Matt Krick <[email protected]>
Signed-off-by: Matt Krick <[email protected]>
Signed-off-by: Matt Krick <[email protected]>
Signed-off-by: Matt Krick <[email protected]>
Signed-off-by: Matt Krick <[email protected]>
Signed-off-by: Matt Krick <[email protected]>
Signed-off-by: Matt Krick <[email protected]>
Signed-off-by: Matt Krick <[email protected]>
Signed-off-by: Matt Krick <[email protected]>
Signed-off-by: Matt Krick <[email protected]>
Signed-off-by: Matt Krick <[email protected]>
Signed-off-by: Matt Krick <[email protected]>
Signed-off-by: Matt Krick <[email protected]>
WalkthroughThe recent changes in the codebase involve transitioning from RethinkDB to PostgreSQL for database operations. This includes refactoring imports, updating database queries, and modifying function logic across multiple files. Additionally, new utility functions and migration scripts were introduced to support these changes. The primary aim is to standardize database interactions to enhance performance and maintainability. Changes
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (2)
Files skipped from review as they are similar to previous changes (1)
Additional comments not posted (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Outside diff range, codebase verification and nitpick comments (34)
packages/server/postgres/migrations/1720640784862_OrganizationUser-phase2.ts (8)
1-4: Remove unused imports.The imports for
randconnectRethinkDBare not used in the function and should be removed to clean up the code.- import {r} from 'rethinkdb-ts' - import connectRethinkDB from '../../database/connectRethinkDB'
7-7: Remove unused function call.The call to
connectRethinkDB()is unnecessary as the function does not use RethinkDB directly.- await connectRethinkDB()
14-14: Remove debug logging.The console.log statement should be removed or replaced with a proper logging mechanism.
- console.log('Adding index')
23-23: Remove hardcoded values.Avoid using hardcoded values like 'aGhostOrganizationUser'. Use a constant or configuration instead.
- .get('aGhostOrganizationUser') + .get(GHOST_ORG_USER_ID)
24-24: Remove debug logging.The console.log statement should be removed or replaced with a proper logging mechanism.
- await console.log('Adding index complete') + console.log('Adding index complete')
43-44: Use descriptive variable names.Variables like
curjoinedAtandcurIdshould be renamed to more descriptive names for better readability.- let curjoinedAt = r.minval - let curId = r.minval + let currentJoinedAt = r.minval + let currentId = r.minval
46-46: Remove debug logging.The console.log statement should be removed or replaced with a proper logging mechanism.
- console.log('inserting row', i * BATCH_SIZE, curjoinedAt, curId)
97-99: Remove unused function call.The call to
connectRethinkDB()is unnecessary as the function does not use RethinkDB directly.- await connectRethinkDB()packages/server/graphql/mutations/helpers/removeFromOrg.ts (2)
Line range hint
8-8: Remove unused import.The import
getRethinkis not used in the function and should be removed to clean up the code.- import getRethink from '../../../database/rethinkDriver'
Line range hint
88-88: Remove debug logging.The console.log statement should be removed or replaced with a proper logging mechanism.
- Logger.log(e) + Logger.error('Adjust user count failed:', e)packages/server/graphql/mutations/oldUpgradeToTeamTier.ts (2)
Line range hint
47-47: Handle potential missingorganization.Ensure that
organizationis not null or undefined before accessing its properties.- const {stripeSubscriptionId: startingSubId, name: orgName, activeDomain: domain} = await dataLoader.get('organizations').loadNonNull(orgId) + const organization = await dataLoader.get('organizations').loadNonNull(orgId) + const {stripeSubscriptionId: startingSubId, name: orgName, activeDomain: domain} = organization || {}
Line range hint
69-69: Remove debug logging.The console.log statement should be removed or replaced with a proper logging mechanism.
- console.log(errors) + Logger.error('Errors during upgrade:', errors)packages/server/graphql/mutations/archiveOrganization.ts (2)
Line range hint
65-65: Handle potential missingorganization.Ensure that
organizationis not null or undefined before accessing its properties.- const {tier} = organization + const organization = await dataLoader.get('organizations').loadNonNull(orgId) + const {tier} = organization || {}
Line range hint
95-95: Remove debug logging.The console.log statement should be removed or replaced with a proper logging mechanism.
- console.log(errors) + Logger.error('Errors during archiving:', errors)packages/server/graphql/public/mutations/setOrgUserRole.ts (1)
Line range hint
9-14:
Consider migrating to PostgreSQL.The function
addNotificationsstill uses RethinkDB. For consistency and maintainability, consider migrating this function to use PostgreSQL.- const r = await getRethink() - await r.table('Notification').insert(promotionNotification).run() + const pg = getKysely() + await pg.insertInto('Notification').values(promotionNotification).execute()packages/server/graphql/private/mutations/draftEnterpriseInvoice.ts (3)
Line range hint
10-13:
Enhance error handling.Consider specifying the error type to provide more context.
- throw new Error('User for email not found') + throw new UserNotFoundError('User for email not found')
Line range hint
35-37:
Simplify conditional checks.Combine the conditions to reduce redundancy.
- if (!userId && !email) { + if (!(userId || email)) {
Line range hint
51-52:
Improve error message clarity.Provide more context in the error message.
- return {error: {message: 'Invalid orgId'}} + return {error: {message: `Invalid orgId: ${orgId}`}}packages/server/billing/helpers/adjustUserCount.ts (4)
Line range hint
69-70:
Clear data loader cache after operations.Ensure data loader cache is cleared after database operations to prevent stale data.
+ dataLoader.get('organizations').clear(orgIds)
Line range hint
89-93:
Improve readability with destructuring.Use destructuring to improve readability.
- .set({removedAt: sql`CURRENT_TIMESTAMP`}) + .set({removedAt: sql`CURRENT_TIMESTAMP()`})
Line range hint
120-121:
Handle potential null user.Ensure the function handles potential null values for the user.
- const user = (await getUserById(userId))! + const user = await getUserById(userId) + if (!user) throw new Error(`User not found: ${userId}`)
Line range hint
129-130:
Clear data loader cache after operations.Ensure data loader cache is cleared after database operations to prevent stale data.
+ dataLoader.get('organizations').clear(orgIds)packages/server/billing/helpers/teamLimitsCheck.ts (7)
Line range hint
14-21:
Optimize database updates.Combine the two database update operations into a single transaction to improve performance.
const pg = getKysely() await pg.transaction().execute(async (trx) => { await trx .updateTable('OrganizationUser') .set({suggestedTier: 'team'}) .where('orgId', '=', orgId) .where('userId', 'in', userIds) .where('removedAt', 'is', null) .execute() await trx .updateTable('User') .set({featureFlags: sql`arr_append_uniq("featureFlags", 'insights')`}) .where('id', 'in', userIds) .execute() })
Line range hint
24-29:
Ensure data consistency with transactions.Wrap the database operations in a transaction to ensure data consistency.
const operationId = dataLoader.share() const subOptions = {operationId} const notificationsToInsert = userIds.map((userId) => { return new NotificationTeamsLimitExceeded({ userId, orgId, orgName, orgPicture }) }) await r.transaction(async (trx) => { await trx.table('Notification').insert(notificationsToInsert).run() notificationsToInsert.forEach((notification) => { publishNotification(notification, subOptions) }) })
Line range hint
36-40:
Optimize database queries.Combine the two database queries into a single query to improve performance.
const teamIds = await getTeamIdsByOrgIds([orgId]) if (teamIds.length <= Threshold.MAX_STARTER_TIER_TEAMS) { return false } const activeTeamCount = await getActiveTeamCountByTeamIds(teamIds) return activeTeamCount >= Threshold.MAX_STARTER_TIER_TEAMS
Line range hint
47-49:
Optimize conditional checks.Combine the conditions to reduce redundancy.
if (!organization.tierLimitExceededAt) { return } + if (!organization.tierLimitExceededAt || !(await isLimitExceeded(orgId))) { + return + }
Line range hint
57-58:
Clear data loader cache after operations.Ensure data loader cache is cleared after database operations to prevent stale data.
+ dataLoader.get('organizations').clear(orgId)
Line range hint
67-68:
Optimize conditional checks.Combine the conditions to reduce redundancy.
if (!featureFlags?.includes('teamsLimit')) return if (tierLimitExceededAt || getFeatureTier({tier, trialStartDate}) !== 'starter') return
Line range hint
92-93:
Clear data loader cache after operations.Ensure data loader cache is cleared after database operations to prevent stale data.
+ dataLoader.get('organizations').clear(orgId)packages/server/graphql/private/mutations/hardDeleteUser.ts (4)
Line range hint
12-13:
Remove unused import.The import
getRethinkis no longer used and should be removed.- import getRethink from '../../../database/rethinkDriver'
Line range hint
51-52:
Improve error message clarity.Provide more context in the error message.
- return {error: {message: 'Invalid orgId'}} + return {error: {message: `Invalid orgId: ${orgId}`}}
Line range hint
90-91:
Combine database operations into a transaction.Wrap the database operations in a transaction to ensure data consistency.
await r.transaction(async (trx) => { await trx({ teamMember: r.table('TeamMember').getAll(userIdToDelete, {index: 'userId'}).delete(), meetingMember: r.table('MeetingMember').getAll(userIdToDelete, {index: 'userId'}).delete(), notification: r.table('Notification').getAll(userIdToDelete, {index: 'userId'}).delete(), suggestedAction: r.table('SuggestedAction').getAll(userIdToDelete, {index: 'userId'}).delete(), createdTasks: r .table('Task') .getAll(r.args(teamIds), {index: 'teamId'}) .filter((row: RValue) => row('createdBy').eq(userIdToDelete)) .delete(), agendaItem: r .table('AgendaItem') .getAll(r.args(teamIds), {index: 'teamId'}) .filter((row: RValue) => r(teamMemberIds).contains(row('teamMemberId'))) .delete(), pushInvitation: r.table('PushInvitation').getAll(userIdToDelete, {index: 'userId'}).delete(), slackNotification: r .table('SlackNotification') .getAll(userIdToDelete, {index: 'userId'}) .delete(), invitedByTeamInvitation: r .table('TeamInvitation') .getAll(r.args(teamIds), {index: 'teamId'}) .filter((row: RValue) => row('invitedBy').eq(userIdToDelete)) .delete(), createdByTeamInvitations: r .table('TeamInvitation') .getAll(r.args(teamIds), {index: 'teamId'}) .filter((row: RValue) => row('acceptedBy').eq(userIdToDelete)) .update({acceptedBy: ''}), comment: r .table('Comment') .getAll(r.args(teamDiscussionIds), {index: 'discussionId'}) .filter((row: RValue) => row('createdBy').eq(userIdToDelete)) .update({ createdBy: tombstoneId, isAnonymous: true }), onePersonMeetings: r .table('NewMeeting') .getAll(r.args(onePersonMeetingIds), {index: 'id'}) .delete(), swapFacilitator: r(swapFacilitatorUpdates).forEach((update) => r .table('NewMeeting') .get(update('id')) .update({ facilitatorUserId: update('otherTeamMember') as unknown as string }) ), swapCreatedByUser: r(swapCreatedByUserUpdates).forEach((update) => r .table('NewMeeting') .get(update('id')) .update({ createdBy: update('otherTeamMember') as unknown as string }) ) }).run() })
Line range hint
124-125:
Combine database operations into a transaction.Wrap the database operations in a transaction to ensure data consistency.
await pg.transaction(async (trx) => { await Promise.all([ trx.query(`DELETE FROM "AtlassianAuth" WHERE "userId" = $1`, [userIdToDelete]), trx.query(`DELETE FROM "GitHubAuth" WHERE "userId" = $1`, [userIdToDelete]), trx.query( `DELETE FROM "TaskEstimate" WHERE "meetingId" = ANY($1::varchar[]) AND "userId" = $2`, [meetingIds, userIdToDelete] ), trx.query( `DELETE FROM "Poll" WHERE "discussionId" = ANY($1::varchar[]) AND "createdById" = $2`, [teamDiscussionIds, userIdToDelete] ) ]) })packages/server/utils/__tests__/isRequestToJoinDomainAllowed.test.ts (1)
9-9: Verify the correct import ofOrganizationUser.The
OrganizationUsertype is exported frompackages/server/postgres/types/index.d.ts. Ensure that the import path in the test file is updated to reflect this.
- File:
packages/server/utils/__tests__/isRequestToJoinDomainAllowed.test.ts- Line:
9Correct Import:
import { OrganizationUser } from '../../postgres/types/index.d.ts';Please update the import statement to ensure correctness.
Analysis chain
Verify the correct import of
OrganizationUser.The import statement for
OrganizationUserhas been updated. Ensure that the module path and the imported entity are correct.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence and correctness of the `OrganizationUser` import. # Test: Check if the `OrganizationUser` is exported from `postgres/types/index`. rg --type ts --pattern 'export .* OrganizationUser' packages/server/postgres/types/index.tsLength of output: 505
Script:
#!/bin/bash # Description: Verify the existence and correctness of the `OrganizationUser` import. # Test: Check if the `OrganizationUser` is exported from `postgres/types/index.ts`. rg 'export .* OrganizationUser' packages/server/postgres/types/index.tsLength of output: 153
Script:
#!/bin/bash # Description: Locate the file that exports `OrganizationUser`. # Step 1: Find all TypeScript files in the repository. fd --type f --extension ts > ts_files.txt # Step 2: Search for the export statement of `OrganizationUser` in the found TypeScript files. rg 'export .* OrganizationUser' $(cat ts_files.txt)Length of output: 130369
| }) | ||
| ) | ||
| return {organizationUsersAffected: recordsReplaced.reduce((x, y) => x + y)} | ||
| return {organizationUsersAffected: recordsReplaced.flat().length} |
There was a problem hiding this comment.
Optimise map to avoid unnecessary nesting.
The Promise.all call can be optimized by using flatMap to avoid unnecessary nesting.
- const recordsReplaced = await Promise.all(
- userIds.map(async (userId) => {
- try {
- return await pg
- .updateTable('OrganizationUser')
- .set({suggestedTier})
- .where('userId', '=', userId)
- .where('orgId', 'in', orgIds)
- .where('removedAt', 'is', null)
- .returning('id')
- .execute()
- } catch (error) {
- Logger.error(`Error updating OrganizationUser for userId ${userId}: ${error.message}`)
- return null
- }
- })
- )
+ const recordsReplaced = await Promise.all(
+ userIds.flatMap(async (userId) => {
+ try {
+ return await pg
+ .updateTable('OrganizationUser')
+ .set({suggestedTier})
+ .where('userId', '=', userId)
+ .where('orgId', 'in', orgIds)
+ .where('removedAt', 'is', null)
+ .returning('id')
+ .execute()
+ } catch (error) {
+ Logger.error(`Error updating OrganizationUser for userId ${userId}: ${error.message}`)
+ return null
+ }
+ })
+ )Committable suggestion was skipped due to low confidence.
| await Promise.allSettled( | ||
| results.map(async ({group: userId, reduction: orgIds}) => { | ||
| results.map(async ({userId, orgIds}) => { |
There was a problem hiding this comment.
Improve error handling within Promise.allSettled.
The error handling within Promise.allSettled can be improved by logging the specific userId and orgIds that caused the error.
- results.map(async ({userId, orgIds}) => {
- try {
- return await adjustUserCount(userId, orgIds, InvoiceItemType.AUTO_PAUSE_USER, dataLoader)
- } catch (e) {
- Logger.warn(`Error adjusting user count`)
- }
- return undefined
- })
+ results.map(async ({userId, orgIds}) => (
+ adjustUserCount(userId, orgIds, InvoiceItemType.AUTO_PAUSE_USER, dataLoader).catch((error) => {
+ Logger.warn(`Error adjusting user count for userId ${userId} and orgIds ${orgIds}: ${error.message}`)
+ })
+ ))Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| results.map(async ({userId, orgIds}) => { | |
| results.map(async ({userId, orgIds}) => ( | |
| adjustUserCount(userId, orgIds, InvoiceItemType.AUTO_PAUSE_USER, dataLoader).catch((error) => { | |
| Logger.warn(`Error adjusting user count for userId ${userId} and orgIds ${orgIds}: ${error.message}`) | |
| }) | |
| )) |
| } catch { | ||
| // index already exists | ||
| } |
There was a problem hiding this comment.
Improve error handling.
Catching generic errors without logging them can make debugging difficult. Log the error message.
- } catch {
+ } catch (e) {
+ console.error('Index creation failed:', e)Committable suggestion was skipped due to low confidence.
| await r.table('OrganizationUser').indexDrop('joinedAtId').run() | ||
| } catch { | ||
| // index already dropped | ||
| } |
There was a problem hiding this comment.
Improve error handling.
Catching generic errors without logging them can make debugging difficult. Log the error message.
- } catch {
+ } catch (e) {
+ console.error('Index drop failed:', e)Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await r.table('OrganizationUser').indexDrop('joinedAtId').run() | |
| } catch { | |
| // index already dropped | |
| } | |
| await r.table('OrganizationUser').indexDrop('joinedAtId').run() | |
| } catch (e) { | |
| console.error('Index drop failed:', e) | |
| // index already dropped | |
| } |
| await Promise.all( | ||
| rowsToInsert.map(async (row) => { |
There was a problem hiding this comment.
Optimize error handling.
Logging errors within a loop can be optimized by aggregating errors and logging them at once.
- await Promise.all(
- rowsToInsert.map(async (row) => {
- try {
- await pg
- .insertInto('OrganizationUser')
- .values(row)
- .onConflict((oc) => oc.doNothing())
- .execute()
- } catch (e) {
- if (e.constraint === 'fk_userId' || e.constraint === 'fk_orgId') {
- console.log(`Skipping ${row.id} because it has no user/org`)
- return
- }
- console.log(e, row)
- }
- })
- )
+ const errors = []
+ for (const row of rowsToInsert) {
+ try {
+ await pg
+ .insertInto('OrganizationUser')
+ .values(row)
+ .onConflict((oc) => oc.doNothing())
+ .execute()
+ } catch (e) {
+ if (e.constraint === 'fk_userId' || e.constraint === 'fk_orgId') {
+ console.log(`Skipping ${row.id} because it has no user/org`)
+ continue
+ }
+ errors.push({error: e, row})
+ }
+ }
+ if (errors.length > 0) {
+ console.log(errors)
+ }Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await Promise.all( | |
| rowsToInsert.map(async (row) => { | |
| const errors = [] | |
| for (const row of rowsToInsert) { | |
| try { | |
| await pg | |
| .insertInto('OrganizationUser') | |
| .values(row) | |
| .onConflict((oc) => oc.doNothing()) | |
| .execute() | |
| } catch (e) { | |
| if (e.constraint === 'fk_userId' || e.constraint === 'fk_orgId') { | |
| console.log(`Skipping ${row.id} because it has no user/org`) | |
| continue | |
| } | |
| errors.push({error: e, row}) | |
| } | |
| } | |
| if (errors.length > 0) { | |
| console.log(errors) | |
| } |
| pg | ||
| .updateTable('OrganizationUser') | ||
| .set({removedAt: sql`CURRENT_TIMESTAMP`}) | ||
| .where('userId', '=', userId) | ||
| .where('orgId', '=', orgId) | ||
| .where('removedAt', 'is', null) | ||
| .returning('role') | ||
| .returning(['id', 'role']) |
There was a problem hiding this comment.
Handle potential missing organizationUser.
Ensure that organizationUser is not null or undefined before accessing its properties.
- const {role} = organizationUser
+ const {role} = organizationUser || {}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .returning(['id', 'role']) | |
| .returning(['id', 'role']) | |
| const {role} = organizationUser || {} |
| const pg = getKysely() | ||
| if (email) { |
There was a problem hiding this comment.
Improve conditional readability.
Use early return to reduce nesting.
- if (!organizationUser) {
- throw new Error('Email not associated with a user on that org')
- }
+ if (!organizationUser) throw new Error('Email not associated with a user on that org')Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const pg = getKysely() | |
| if (email) { | |
| const pg = getKysely() | |
| if (!organizationUser) throw new Error('Email not associated with a user on that org') | |
| if (email) { |
Signed-off-by: Matt Krick <[email protected]>
Signed-off-by: Matt Krick <[email protected]>
Description
Remove reads from RethinkDB
TEST
Summary by CodeRabbit
New Features
OrganizationUsertable, including index creation and data manipulation.Refactor
Bug Fixes
OrganizationUserrecords.Tests