chore(rethinkdb): TeamMember: Phase 2 #9993
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Matt Krick <matt.krick@gmail.com>
Signed-off-by: Matt Krick <matt.krick@gmail.com>
Signed-off-by: Matt Krick <matt.krick@gmail.com>
Signed-off-by: Matt Krick <matt.krick@gmail.com>
Signed-off-by: Matt Krick <matt.krick@gmail.com>
Signed-off-by: Matt Krick <matt.krick@gmail.com>
Signed-off-by: Matt Krick <matt.krick@gmail.com>
Signed-off-by: Matt Krick <matt.krick@gmail.com>
Signed-off-by: Matt Krick <matt.krick@gmail.com>
|
This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR will be delayed and might be rejected due to its size. |
WalkthroughThe recent updates enhance data handling and retrieval mechanisms across the client and server packages. Key improvements include the introduction of default values for user names, the flexibility of nullable fields in the database, and optimised data loading through DataLoader. Additionally, a significant transition from RethinkDB to PostgreSQL for various operations aims to boost reliability, performance, and maintainability. Overall, these changes streamline data access and ensure consistent defaults. Changes
Sequence Diagram(s)The changes predominantly focus on data handling and enhancements across various modules. As these updates do not introduce new control flows or features requiring visual representation, no sequence diagrams are generated. Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (4)
Files skipped from review due to trivial changes (4)
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 Configuration 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: 8
Outside diff range, codebase verification and nitpick comments (5)
packages/server/utils/authorization.ts (1)
Line range hint
15-24:
Add error handling for database queries.The function should handle potential errors from the database queries to ensure robustness.
export const isTeamLead = async (userId: string, teamId: string, dataLoader: DataLoaderWorker) => { try { const r = await getRethink() const teamMemberId = toTeamMemberId(teamId, userId) if (await r.table('TeamMember').get(teamMemberId)('isLead').default(false).run()) { return true } const team = await dataLoader.get('teams').loadNonNull(teamId) const organizationUser = await dataLoader .get('organizationUsersByUserIdOrgId') .load({userId, orgId: team.orgId}) return organizationUser?.role === 'ORG_ADMIN' } catch (error) { console.error('Error checking if user is team lead:', error) return false } }packages/server/graphql/public/mutations/updateUserProfile.ts (1)
53-70: Inconsistent usage of PostgreSQL and RethinkDB in application logicThe codebase shows extensive usage of RethinkDB in the application logic, while PostgreSQL usage is limited to configuration files. This inconsistency may lead to potential issues in data management and synchronisation.
- Ensure that PostgreSQL is integrated consistently in the application logic where required.
- Review the database operations to maintain consistency and avoid potential data issues.
Analysis chain
PostgreSQL query execution and data update logic looks good.
The logic correctly updates the
preferredNamein both PostgreSQL and RethinkDB. Ensure consistency in using both databases.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of both PostgreSQL and RethinkDB in the codebase. # Test: Search for the usage of both databases. Expect: Consistent usage. rg --type python -A 5 $'getKysely' rg --type python -A 5 $'getRethink'Length of output: 130
Script:
#!/bin/bash # Description: Verify the usage of both PostgreSQL and RethinkDB in the codebase. # Test: Search for the usage of both databases. Expect: Consistent usage. rg --type js -A 5 $'getKysely' rg --type js -A 5 $'getRethink'Length of output: 1398
Script:
#!/bin/bash # Description: Verify the usage of both PostgreSQL and RethinkDB in the codebase. # Test: Search for the usage of both databases. Expect: Consistent usage. rg --type js -A 5 $'pg' rg --type js -A 5 $'r.table'Length of output: 62525
Script:
#!/bin/bash # Description: Verify the usage of PostgreSQL in the application logic. # Test: Search for the usage of PostgreSQL in TypeScript/JavaScript files, excluding configuration and migration scripts. rg --type js -g '!scripts/**' -g '!packages/server/database/migrations/**' -A 5 $'pg'Length of output: 1509
packages/server/graphql/resolvers.ts (2)
Line range hint
79-83:
Ensure proper type checking forteamIdandteam.The function
resolveTeamshould include type checks forteamIdandteamto ensure they are valid.export const resolveTeam = ( {team, teamId}: {teamId?: string; team?: TeamSource}, _args: unknown, {dataLoader}: GQLContext ) => { if (teamId && typeof teamId !== 'string') { throw new Error('Invalid teamId') } if (team && typeof team !== 'object') { throw new Error('Invalid team') } // TODO figure out how to lock this down without using the tms, since the mutation may have invalidated it return teamId ? dataLoader.get('teams').load(teamId) : team // const teamDoc = teamId ? await dataLoader.get('teams').load(teamId) : team; // const {tms} = authToken; // return tms.includes(teamDoc.id) ? teamDoc : null; }
Line range hint
91-95:
Ensure proper type checking forteamIdsandteams.The function
resolveTeamsshould include type checks forteamIdsandteamsto ensure they are valid.export const resolveTeams = ( {teamIds, teams}: {teamIds: string; teams: TeamSource[]}, _args: unknown, {dataLoader}: GQLContext ) => { if (teamIds && !Array.isArray(teamIds)) { throw new Error('Invalid teamIds') } if (teams && !Array.isArray(teams)) { throw new Error('Invalid teams') } // TODO figure out how to lock this down without using the tms, since the mutation may have invalidated it return teamIds && teamIds.length > 0 ? dataLoader.get('teams').loadMany(teamIds) : teams // const {tms} = authToken; // const teamDocs = (teamIds && teamIds.length > 0) ? await dataLoader.get('teams').loadMany(teamIds) : teams; // return Array.isArray(teamDocs) ? teamDocs.filter((team) => tms.includes(team.id)) : null; }packages/server/graphql/private/typeDefs/_legacy.graphql (1)
Issues found due to the removal of the
NewMeetinginterface.The
NewMeetinginterface is extensively used across the codebase, including in GraphQL type definitions, mutations, queries, and various other files. Its removal will cause numerous broken references and errors.
- File:
packages/server/graphql/private/typeDefs/_legacy.graphql- Lines: 756-761
Please review and address the impacted areas to ensure the codebase remains functional.
Analysis chain
Line range hint
1-1:
Verify the impact of removing theNewMeetinginterface.The removal of the
NewMeetinginterface may impact other parts of the codebase that depend on this interface.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the impact of removing the `NewMeeting` interface in the codebase. # Test: Search for the usage of `NewMeeting` interface. Expect: No broken references in the codebase. rg --type graphql 'NewMeeting'Length of output: 62
Script:
#!/bin/bash # Description: Verify the impact of removing the `NewMeeting` interface in the codebase. # Test: Search for the usage of `NewMeeting` interface across all files. Expect: No broken references in the codebase. rg 'NewMeeting'Length of output: 107587
Comment on lines
35
to
40
| .forEach((row: any) => { | ||
| return r | ||
| .table('TeamMember') | ||
| .get(row('id')) | ||
| .update({updatedAt: row('updatedAt')}) | ||
| }) |
There was a problem hiding this comment.
Handle potential errors in the forEach loop.
The forEach loop should handle potential errors that may occur during the update operation.
- await r(dateDates)
- .forEach((row: any) => {
- return r
- .table('TeamMember')
- .get(row('id'))
- .update({updatedAt: row('updatedAt')})
- })
- .run()
+ await Promise.all(
+ dateDates.map(async (row: any) => {
+ try {
+ await r
+ .table('TeamMember')
+ .get(row.id)
+ .update({updatedAt: row.updatedAt})
+ .run()
+ } catch (error) {
+ console.error(`Failed to update date for row ${row.id}:`, error)
+ }
+ })
+ )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.
Suggested change
| .forEach((row: any) => { | |
| return r | |
| .table('TeamMember') | |
| .get(row('id')) | |
| .update({updatedAt: row('updatedAt')}) | |
| }) | |
| await Promise.all( | |
| dateDates.map(async (row: any) => { | |
| try { | |
| await r | |
| .table('TeamMember') | |
| .get(row.id) | |
| .update({updatedAt: row.updatedAt}) | |
| .run() | |
| } catch (error) { | |
| console.error(`Failed to update date for row ${row.id}:`, error) | |
| } | |
| }) | |
| ) |
Comment on lines
14
to
15
| // add a dummy date for nulls | ||
| const parabolEpoch = new Date('2016-06-01') |
There was a problem hiding this comment.
Consider making the dummy date configurable.
Hardcoding the dummy date can lead to issues if the date needs to be changed in the future.
- const parabolEpoch = new Date('2016-06-01')
+ const PARABOL_EPOCH = process.env.PARABOL_EPOCH || '2016-06-01'
+ const parabolEpoch = new Date(PARABOL_EPOCH)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.
Suggested change
| // add a dummy date for nulls | |
| const parabolEpoch = new Date('2016-06-01') | |
| // add a dummy date for nulls | |
| const PARABOL_EPOCH = process.env.PARABOL_EPOCH || '2016-06-01' | |
| const parabolEpoch = new Date(PARABOL_EPOCH) |
Comment on lines
103
to
127
| try { | ||
| await pg | ||
| .insertInto('TeamMember') | ||
| .values(rowsToInsert) | ||
| .onConflict((oc) => oc.doNothing()) | ||
| .execute() | ||
| } catch (e) { | ||
| await Promise.all( | ||
| rowsToInsert.map(async (row) => { | ||
| try { | ||
| await pg | ||
| .insertInto('TeamMember') | ||
| .values(row) | ||
| .onConflict((oc) => oc.doNothing()) | ||
| .execute() | ||
| } catch (e) { | ||
| if (e.constraint === 'fk_userId' || e.constraint === 'fk_teamId') { | ||
| console.log(`Skipping ${row.id} because it has no user/team`) | ||
| return | ||
| } | ||
| console.log(e, row) | ||
| } | ||
| }) | ||
| ) | ||
| } |
There was a problem hiding this comment.
Improve error handling for batch insert operations.
The batch insert operation should handle potential errors more gracefully and provide more detailed logging.
- try {
- await pg
- .insertInto('TeamMember')
- .values(rowsToInsert)
- .onConflict((oc) => oc.doNothing())
- .execute()
- } catch (e) {
- await Promise.all(
- rowsToInsert.map(async (row) => {
- try {
- await pg
- .insertInto('TeamMember')
- .values(row)
- .onConflict((oc) => oc.doNothing())
- .execute()
- } catch (e) {
- if (e.constraint === 'fk_userId' || e.constraint === 'fk_teamId') {
- console.log(`Skipping ${row.id} because it has no user/team`)
- return
- }
- console.log(e, row)
- }
- })
- )
- }
+ try {
+ await pg
+ .insertInto('TeamMember')
+ .values(rowsToInsert)
+ .onConflict((oc) => oc.doNothing())
+ .execute()
+ } catch (error) {
+ console.error('Batch insert failed:', error)
+ await Promise.all(
+ rowsToInsert.map(async (row) => {
+ try {
+ await pg
+ .insertInto('TeamMember')
+ .values(row)
+ .onConflict((oc) => oc.doNothing())
+ .execute()
+ } catch (e) {
+ if (e.constraint === 'fk_userId' || e.constraint === 'fk_teamId') {
+ console.log(`Skipping ${row.id} because it has no user/team`)
+ return
+ }
+ console.error(`Failed to insert row ${row.id}:`, 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.
Suggested change
| try { | |
| await pg | |
| .insertInto('TeamMember') | |
| .values(rowsToInsert) | |
| .onConflict((oc) => oc.doNothing()) | |
| .execute() | |
| } catch (e) { | |
| await Promise.all( | |
| rowsToInsert.map(async (row) => { | |
| try { | |
| await pg | |
| .insertInto('TeamMember') | |
| .values(row) | |
| .onConflict((oc) => oc.doNothing()) | |
| .execute() | |
| } catch (e) { | |
| if (e.constraint === 'fk_userId' || e.constraint === 'fk_teamId') { | |
| console.log(`Skipping ${row.id} because it has no user/team`) | |
| return | |
| } | |
| console.log(e, row) | |
| } | |
| }) | |
| ) | |
| } | |
| try { | |
| await pg | |
| .insertInto('TeamMember') | |
| .values(rowsToInsert) | |
| .onConflict((oc) => oc.doNothing()) | |
| .execute() | |
| } catch (error) { | |
| console.error('Batch insert failed:', error) | |
| await Promise.all( | |
| rowsToInsert.map(async (row) => { | |
| try { | |
| await pg | |
| .insertInto('TeamMember') | |
| .values(row) | |
| .onConflict((oc) => oc.doNothing()) | |
| .execute() | |
| } catch (e) { | |
| if (e.constraint === 'fk_userId' || e.constraint === 'fk_teamId') { | |
| console.log(`Skipping ${row.id} because it has no user/team`) | |
| return | |
| } | |
| console.error(`Failed to insert row ${row.id}:`, e) | |
| } | |
| }) | |
| ) | |
| } |
Comment on lines
13
to
41
| const setFacilitatedUserIdOrDelete = async ( | ||
| userIdToDelete: string, | ||
| teamIds: string[], | ||
| dataLoader: DataLoaderInstance | ||
| ) => { | ||
| const r = await getRethink() | ||
| const facilitatedMeetings = await r | ||
| .table('NewMeeting') | ||
| .getAll(r.args(teamIds), {index: 'teamId'}) | ||
| .filter((row: RValue) => row('createdBy').eq(userIdToDelete)) | ||
| .run() | ||
| facilitatedMeetings.map(async (meeting) => { | ||
| const {id: meetingId} = meeting | ||
| const meetingMembers = await dataLoader.get('meetingMembersByMeetingId').load(meetingId) | ||
| const otherMember = meetingMembers.find(({userId}) => userId !== userIdToDelete) | ||
| if (otherMember) { | ||
| await r | ||
| .table('NewMeeting') | ||
| .get(meetingId) | ||
| .update({ | ||
| facilitatorUserId: otherMember.userId | ||
| }) | ||
| .run() | ||
| } else { | ||
| // single-person meeting must be deleted because facilitatorUserId must be non-null | ||
| await r.table('NewMeeting').get(meetingId).delete().run() | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
Ensure proper error handling for asynchronous operations.
The function setFacilitatedUserIdOrDelete performs multiple asynchronous operations without error handling. Consider adding try-catch blocks to handle potential errors.
const setFacilitatedUserIdOrDelete = async (
userIdToDelete: string,
teamIds: string[],
dataLoader: DataLoaderInstance
) => {
try {
const r = await getRethink()
const facilitatedMeetings = await r
.table('NewMeeting')
.getAll(r.args(teamIds), {index: 'teamId'})
.filter((row: RValue) => row('createdBy').eq(userIdToDelete))
.run()
facilitatedMeetings.map(async (meeting) => {
const {id: meetingId} = meeting
const meetingMembers = await dataLoader.get('meetingMembersByMeetingId').load(meetingId)
const otherMember = meetingMembers.find(({userId}) => userId !== userIdToDelete)
if (otherMember) {
await r
.table('NewMeeting')
.get(meetingId)
.update({
facilitatorUserId: otherMember.userId
})
.run()
} else {
// single-person meeting must be deleted because facilitatorUserId must be non-null
await r.table('NewMeeting').get(meetingId).delete().run()
}
})
} catch (error) {
console.error('Error in setFacilitatedUserIdOrDelete:', error)
throw error
}
}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.
Suggested change
| const setFacilitatedUserIdOrDelete = async ( | |
| userIdToDelete: string, | |
| teamIds: string[], | |
| dataLoader: DataLoaderInstance | |
| ) => { | |
| const r = await getRethink() | |
| const facilitatedMeetings = await r | |
| .table('NewMeeting') | |
| .getAll(r.args(teamIds), {index: 'teamId'}) | |
| .filter((row: RValue) => row('createdBy').eq(userIdToDelete)) | |
| .run() | |
| facilitatedMeetings.map(async (meeting) => { | |
| const {id: meetingId} = meeting | |
| const meetingMembers = await dataLoader.get('meetingMembersByMeetingId').load(meetingId) | |
| const otherMember = meetingMembers.find(({userId}) => userId !== userIdToDelete) | |
| if (otherMember) { | |
| await r | |
| .table('NewMeeting') | |
| .get(meetingId) | |
| .update({ | |
| facilitatorUserId: otherMember.userId | |
| }) | |
| .run() | |
| } else { | |
| // single-person meeting must be deleted because facilitatorUserId must be non-null | |
| await r.table('NewMeeting').get(meetingId).delete().run() | |
| } | |
| }) | |
| } | |
| const setFacilitatedUserIdOrDelete = async ( | |
| userIdToDelete: string, | |
| teamIds: string[], | |
| dataLoader: DataLoaderInstance | |
| ) => { | |
| try { | |
| const r = await getRethink() | |
| const facilitatedMeetings = await r | |
| .table('NewMeeting') | |
| .getAll(r.args(teamIds), {index: 'teamId'}) | |
| .filter((row: RValue) => row('createdBy').eq(userIdToDelete)) | |
| .run() | |
| facilitatedMeetings.map(async (meeting) => { | |
| const {id: meetingId} = meeting | |
| const meetingMembers = await dataLoader.get('meetingMembersByMeetingId').load(meetingId) | |
| const otherMember = meetingMembers.find(({userId}) => userId !== userIdToDelete) | |
| if (otherMember) { | |
| await r | |
| .table('NewMeeting') | |
| .get(meetingId) | |
| .update({ | |
| facilitatorUserId: otherMember.userId | |
| }) | |
| .run() | |
| } else { | |
| // single-person meeting must be deleted because facilitatorUserId must be non-null | |
| await r.table('NewMeeting').get(meetingId).delete().run() | |
| } | |
| }) | |
| } catch (error) { | |
| console.error('Error in setFacilitatedUserIdOrDelete:', error) | |
| throw error | |
| } | |
| } |
Comment on lines
82
to
89
| await setFacilitatedUserIdOrDelete(userIdToDelete, teamIds, dataLoader) | ||
| await r({ | ||
| nullifyCreatedBy: r | ||
| .table('NewMeeting') | ||
| .getAll(r.args(teamIds), {index: 'teamId'}) | ||
| .filter((row: RValue) => row('createdBy').eq(userIdToDelete)) | ||
| .update({createdBy: null}) | ||
| .run(), |
There was a problem hiding this comment.
Ensure proper error handling for asynchronous operations.
The call to setFacilitatedUserIdOrDelete and subsequent RethinkDB operations lack error handling. Consider adding try-catch blocks to handle potential errors.
await setFacilitatedUserIdOrDelete(userIdToDelete, teamIds, dataLoader)
try {
await r({
nullifyCreatedBy: r
.table('NewMeeting')
.getAll(r.args(teamIds), {index: 'teamId'})
.filter((row: RValue) => row('createdBy').eq(userIdToDelete))
.update({createdBy: null})
.run(),
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
})
}).run()
} catch (error) {
console.error('Error in hardDeleteUser RethinkDB operations:', error)
throw error
}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.
Suggested change
| await setFacilitatedUserIdOrDelete(userIdToDelete, teamIds, dataLoader) | |
| await r({ | |
| nullifyCreatedBy: r | |
| .table('NewMeeting') | |
| .getAll(r.args(teamIds), {index: 'teamId'}) | |
| .filter((row: RValue) => row('createdBy').eq(userIdToDelete)) | |
| .update({createdBy: null}) | |
| .run(), | |
| await setFacilitatedUserIdOrDelete(userIdToDelete, teamIds, dataLoader) | |
| try { | |
| await r({ | |
| nullifyCreatedBy: r | |
| .table('NewMeeting') | |
| .getAll(r.args(teamIds), {index: 'teamId'}) | |
| .filter((row: RValue) => row('createdBy').eq(userIdToDelete)) | |
| .update({createdBy: null}) | |
| .run(), | |
| 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 | |
| }) | |
| }).run() | |
| } catch (error) { | |
| console.error('Error in hardDeleteUser RethinkDB operations:', error) | |
| throw error | |
| } |
| @@ -27,10 +26,11 @@ const removeTeamMember = async ( | |||
| ) => { | |||
| const {evictorUserId} = options | |||
| const r = await getRethink() | |||
| const pg = getKysely() | |||
There was a problem hiding this comment.
Ensure proper error handling for asynchronous operations.
The function removeTeamMember performs multiple asynchronous operations without error handling. Consider adding try-catch blocks to handle potential errors.
const removeTeamMember = async (
teamMemberId: string,
options: Options,
dataLoader: DataLoaderWorker
) => {
const {evictorUserId} = options
const r = await getRethink()
const pg = getKysely()
const now = new Date()
const {userId, teamId} = fromTeamMemberId(teamMemberId)
try {
// see if they were a leader, make a new guy leader so later we can reassign tasks
const activeTeamMembers = await dataLoader.get('teamMembersByTeamId').load(teamId)
const teamMember = activeTeamMembers.find((t) => t.id === teamMemberId)
const {isLead, isNotRemoved} = teamMember ?? {}
// if the guy being removed is the leader & not the last, pick a new one. else, use him
const teamLeader = activeTeamMembers.find((t) => t.isLead === !isLead) || teamMember
if (!isNotRemoved || !teamMember || !teamLeader) {
throw new Error('Team member already removed')
}
if (activeTeamMembers.length === 1) {
await Promise.all([
// archive single-person teams
pg.updateTable('Team').set({isArchived: true}).where('id', '=', teamId).execute(),
// delete all tasks belonging to a 1-person team
r.table('Task').getAll(teamId, {index: 'teamId'}).delete()
])
} else if (isLead) {
await pg
.updateTable('TeamMember')
.set(({not}) => ({isLead: not('isLead')}))
.where('id', 'in', [teamMemberId, teamLeader.id])
.execute()
// assign new leader, remove old leader flag
await r({
newTeamLead: r.table('TeamMember').get(teamLeader.id).update({
isLead: true
}),
oldTeamLead: r.table('TeamMember').get(teamMemberId).update({isLead: false})
}).run()
}
await pg
.updateTable('TeamMember')
.set({isNotRemoved: false})
.where('id', '=', teamMemberId)
.execute()
// assign active tasks to the team lead
const {integratedTasksToArchive, reassignedTasks} = await r({
teamMember: r.table('TeamMember').get(teamMemberId).update({
isNotRemoved: false,
updatedAt: now
}),
integratedTasksToArchive: r
.table('Task')
.getAll(userId, {index: 'userId'})
.filter({teamId})
.filter((task: RDatum) => {
return r.and(
task('tags').contains('archived').not(),
task('integrations').default(null).ne(null)
)
})
.coerceTo('array') as unknown as Task[],
reassignedTasks: r
.table('Task')
.getAll(userId, {index: 'userId'})
.filter({teamId})
.filter((task: RDatum) =>
r.and(task('tags').contains('archived').not(), task('integrations').default(null).eq(null))
)
.update(
{
userId: teamLeader.userId
},
{returnChanges: true}
)('changes')('new_val')
.default([]) as unknown as Task[]
}).run()
await pg
.updateTable('User')
.set(({fn, ref, val}) => ({tms: fn('ARRAY_REMOVE', [ref('tms'), val(teamId)])}))
.where('id', '=', userId)
.execute()
dataLoader.clearAll(['users', 'teamMembers'])
const user = await dataLoader.get('users').load(userId)
let notificationId: string | undefined
if (evictorUserId) {
const notification = new NotificationKickedOut({teamId, userId, evictorUserId})
notificationId = notification.id
await r.table('Notification').insert(notification).run()
}
const archivedTasks = await archiveTasksForDB(integratedTasksToArchive)
const archivedTaskIds = archivedTasks.map(({id}) => id)
const agendaItemIds = await r
.table('AgendaItem')
.getAll(teamId, {index: 'teamId'})
.filter((row: RDatum) => row('teamMemberId').eq(teamMemberId))
.getField('id')
.run()
// if a new meeting was currently running, remove them from it
const filterFn = (stage: CheckInStage | UpdatesStage | EstimateStage | AgendaItemsStage) =>
(stage as CheckInStage | UpdatesStage).teamMemberId === teamMemberId ||
agendaItemIds.includes((stage as AgendaItemsStage).agendaItemId)
removeSlackAuths(userId, teamId)
await removeStagesFromMeetings(filterFn, teamId, dataLoader)
await removeUserFromMeetingStages(userId, teamId, dataLoader)
// TODO should probably just inactivate the meeting member
const activeMeetings = await dataLoader.get('activeMeetingsByTeamId').load(teamId)
const meetingIds = activeMeetings.map(({id}) => id)
await r
.table('MeetingMember')
.getAll(r.args(meetingIds), {index: 'meetingId'})
.filter({userId})
.delete()
.run()
// Reassign facilitator for meetings this user is facilitating.
const facilitatingMeetings = await r
.table('NewMeeting')
.getAll(r.args(meetingIds), {index: 'id'})
.filter({
facilitatorUserId: userId
})
.run()
const newMeetingFacilitators = (
await dataLoader
.get('meetingMembersByMeetingId')
.loadMany(facilitatingMeetings.map((meeting) => meeting.id))
)
.filter(errorFilter)
.map((members) => members[0])
.filter((member) => !!member)
Promise.allSettled(
newMeetingFacilitators.map(async (newFacilitator) => {
if (!newFacilitator) {
// This user is the only meeting member, so do nothing.
// :TODO: (jmtaber129): Consider closing meetings where this user is the only meeting
// member.
return
}
await r
.table('NewMeeting')
.get(newFacilitator.meetingId)
.update({
facilitatorUserId: newFacilitator.userId,
updatedAt: now
})
.run()
})
)
return {
user,
notificationId,
archivedTaskIds,
reassignedTaskIds: reassignedTasks.map(({id}) => id)
}
} catch (error) {
console.error('Error in removeTeamMember:', error)
throw error
}
}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.
Suggested change
| const pg = getKysely() | |
| const removeTeamMember = async ( | |
| teamMemberId: string, | |
| options: Options, | |
| dataLoader: DataLoaderWorker | |
| ) => { | |
| const {evictorUserId} = options | |
| const r = await getRethink() | |
| const pg = getKysely() | |
| const now = new Date() | |
| const {userId, teamId} = fromTeamMemberId(teamMemberId) | |
| try { | |
| // see if they were a leader, make a new guy leader so later we can reassign tasks | |
| const activeTeamMembers = await dataLoader.get('teamMembersByTeamId').load(teamId) | |
| const teamMember = activeTeamMembers.find((t) => t.id === teamMemberId) | |
| const {isLead, isNotRemoved} = teamMember ?? {} | |
| // if the guy being removed is the leader & not the last, pick a new one. else, use him | |
| const teamLeader = activeTeamMembers.find((t) => t.isLead === !isLead) || teamMember | |
| if (!isNotRemoved || !teamMember || !teamLeader) { | |
| throw new Error('Team member already removed') | |
| } | |
| if (activeTeamMembers.length === 1) { | |
| await Promise.all([ | |
| // archive single-person teams | |
| pg.updateTable('Team').set({isArchived: true}).where('id', '=', teamId).execute(), | |
| // delete all tasks belonging to a 1-person team | |
| r.table('Task').getAll(teamId, {index: 'teamId'}).delete() | |
| ]) | |
| } else if (isLead) { | |
| await pg | |
| .updateTable('TeamMember') | |
| .set(({not}) => ({isLead: not('isLead')})) | |
| .where('id', 'in', [teamMemberId, teamLeader.id]) | |
| .execute() | |
| // assign new leader, remove old leader flag | |
| await r({ | |
| newTeamLead: r.table('TeamMember').get(teamLeader.id).update({ | |
| isLead: true | |
| }), | |
| oldTeamLead: r.table('TeamMember').get(teamMemberId).update({isLead: false}) | |
| }).run() | |
| } | |
| await pg | |
| .updateTable('TeamMember') | |
| .set({isNotRemoved: false}) | |
| .where('id', '=', teamMemberId) | |
| .execute() | |
| // assign active tasks to the team lead | |
| const {integratedTasksToArchive, reassignedTasks} = await r({ | |
| teamMember: r.table('TeamMember').get(teamMemberId).update({ | |
| isNotRemoved: false, | |
| updatedAt: now | |
| }), | |
| integratedTasksToArchive: r | |
| .table('Task') | |
| .getAll(userId, {index: 'userId'}) | |
| .filter({teamId}) | |
| .filter((task: RDatum) => { | |
| return r.and( | |
| task('tags').contains('archived').not(), | |
| task('integrations').default(null).ne(null) | |
| ) | |
| }) | |
| .coerceTo('array') as unknown as Task[], | |
| reassignedTasks: r | |
| .table('Task') | |
| .getAll(userId, {index: 'userId'}) | |
| .filter({teamId}) | |
| .filter((task: RDatum) => | |
| r.and(task('tags').contains('archived').not(), task('integrations').default(null).eq(null)) | |
| ) | |
| .update( | |
| { | |
| userId: teamLeader.userId | |
| }, | |
| {returnChanges: true} | |
| )('changes')('new_val') | |
| .default([]) as unknown as Task[] | |
| }).run() | |
| await pg | |
| .updateTable('User') | |
| .set(({fn, ref, val}) => ({tms: fn('ARRAY_REMOVE', [ref('tms'), val(teamId)])})) | |
| .where('id', '=', userId) | |
| .execute() | |
| dataLoader.clearAll(['users', 'teamMembers']) | |
| const user = await dataLoader.get('users').load(userId) | |
| let notificationId: string | undefined | |
| if (evictorUserId) { | |
| const notification = new NotificationKickedOut({teamId, userId, evictorUserId}) | |
| notificationId = notification.id | |
| await r.table('Notification').insert(notification).run() | |
| } | |
| const archivedTasks = await archiveTasksForDB(integratedTasksToArchive) | |
| const archivedTaskIds = archivedTasks.map(({id}) => id) | |
| const agendaItemIds = await r | |
| .table('AgendaItem') | |
| .getAll(teamId, {index: 'teamId'}) | |
| .filter((row: RDatum) => row('teamMemberId').eq(teamMemberId)) | |
| .getField('id') | |
| .run() | |
| // if a new meeting was currently running, remove them from it | |
| const filterFn = (stage: CheckInStage | UpdatesStage | EstimateStage | AgendaItemsStage) => | |
| (stage as CheckInStage | UpdatesStage).teamMemberId === teamMemberId || | |
| agendaItemIds.includes((stage as AgendaItemsStage).agendaItemId) | |
| removeSlackAuths(userId, teamId) | |
| await removeStagesFromMeetings(filterFn, teamId, dataLoader) | |
| await removeUserFromMeetingStages(userId, teamId, dataLoader) | |
| // TODO should probably just inactivate the meeting member | |
| const activeMeetings = await dataLoader.get('activeMeetingsByTeamId').load(teamId) | |
| const meetingIds = activeMeetings.map(({id}) => id) | |
| await r | |
| .table('MeetingMember') | |
| .getAll(r.args(meetingIds), {index: 'meetingId'}) | |
| .filter({userId}) | |
| .delete() | |
| .run() | |
| // Reassign facilitator for meetings this user is facilitating. | |
| const facilitatingMeetings = await r | |
| .table('NewMeeting') | |
| .getAll(r.args(meetingIds), {index: 'id'}) | |
| .filter({ | |
| facilitatorUserId: userId | |
| }) | |
| .run() | |
| const newMeetingFacilitators = ( | |
| await dataLoader | |
| .get('meetingMembersByMeetingId') | |
| .loadMany(facilitatingMeetings.map((meeting) => meeting.id)) | |
| ) | |
| .filter(errorFilter) | |
| .map((members) => members[0]) | |
| .filter((member) => !!member) | |
| Promise.allSettled( | |
| newMeetingFacilitators.map(async (newFacilitator) => { | |
| if (!newFacilitator) { | |
| // This user is the only meeting member, so do nothing. | |
| // :TODO: (jmtaber129): Consider closing meetings where this user is the only meeting | |
| // member. | |
| return | |
| } | |
| await r | |
| .table('NewMeeting') | |
| .get(newFacilitator.meetingId) | |
| .update({ | |
| facilitatorUserId: newFacilitator.userId, | |
| updatedAt: now | |
| }) | |
| .run() | |
| }) | |
| ) | |
| return { | |
| user, | |
| notificationId, | |
| archivedTaskIds, | |
| reassignedTaskIds: reassignedTasks.map(({id}) => id) | |
| } | |
| } catch (error) { | |
| console.error('Error in removeTeamMember:', error) | |
| throw error | |
| } | |
| } |
Comment on lines
48
to
66
| .with('TeamInsert', (qc) => qc.insertInto('Team').values(verifiedTeam)) | ||
| .with('UserUpdate', (qc) => | ||
| qc | ||
| .updateTable('User') | ||
| .set({tms: sql`arr_append_uniq("tms", ${teamId})`}) | ||
| .where('id', '=', userId) | ||
| ) | ||
| .with('TeamMemberInsert', (qc) => | ||
| qc.insertInto('TeamMember').values({ | ||
| id: TeamMemberId.join(teamId, userId), | ||
| teamId, | ||
| userId, | ||
| picture, | ||
| preferredName, | ||
| email, | ||
| isLead: true, | ||
| openDrawer: 'manageTeam' | ||
| }) | ||
| ) |
There was a problem hiding this comment.
Add error handling for database operations.
The function should handle potential errors from the database operations to ensure robustness.
export default async function createTeamAndLeader(
user: IUser,
newTeam: ValidNewTeam,
dataLoader: DataLoaderInstance
) {
try {
const r = await getRethink()
const {id: userId, picture, preferredName, email} = user
const {id: teamId, orgId} = newTeam
const organization = await dataLoader.get('organizations').loadNonNull(orgId)
const {tier, trialStartDate} = organization
const verifiedTeam = new Team({...newTeam, createdBy: userId, tier, trialStartDate})
const meetingSettings = [
new MeetingSettingsRetrospective({teamId}),
new MeetingSettingsAction({teamId}),
new MeetingSettingsPoker({teamId})
]
const timelineEvent = new TimelineEventCreatedTeam({
createdAt: new Date(Date.now() + 5),
userId,
teamId,
orgId
})
const pg = getKysely()
await Promise.all([
pg
.with('TeamInsert', (qc) => qc.insertInto('Team').values(verifiedTeam))
.with('UserUpdate', (qc) =>
qc
.updateTable('User')
.set({tms: sql`arr_append_uniq("tms", ${teamId})`})
.where('id', '=', userId)
)
.with('TeamMemberInsert', (qc) =>
qc.insertInto('TeamMember').values({
id: TeamMemberId.join(teamId, userId),
teamId,
userId,
picture,
preferredName,
email,
isLead: true,
openDrawer: 'manageTeam'
})
)
.insertInto('TimelineEvent')
.values(timelineEvent)
.execute(),
// add meeting settings
r.table('MeetingSettings').insert(meetingSettings).run(),
// denormalize common fields to team member
insertNewTeamMember(user, teamId, dataLoader)
])
dataLoader.clearAll(['teams', 'users', 'teamMembers', 'timelineEvents', 'meetingSettings'])
} catch (error) {
console.error('Error creating team and leader:', error)
}
}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.
Suggested change
| .with('TeamInsert', (qc) => qc.insertInto('Team').values(verifiedTeam)) | |
| .with('UserUpdate', (qc) => | |
| qc | |
| .updateTable('User') | |
| .set({tms: sql`arr_append_uniq("tms", ${teamId})`}) | |
| .where('id', '=', userId) | |
| ) | |
| .with('TeamMemberInsert', (qc) => | |
| qc.insertInto('TeamMember').values({ | |
| id: TeamMemberId.join(teamId, userId), | |
| teamId, | |
| userId, | |
| picture, | |
| preferredName, | |
| email, | |
| isLead: true, | |
| openDrawer: 'manageTeam' | |
| }) | |
| ) | |
| .with('TeamInsert', (qc) => qc.insertInto('Team').values(verifiedTeam)) | |
| .with('UserUpdate', (qc) => | |
| qc | |
| .updateTable('User') | |
| .set({tms: sql`arr_append_uniq("tms", ${teamId})`}) | |
| .where('id', '=', userId) | |
| ) | |
| .with('TeamMemberInsert', (qc) => | |
| qc.insertInto('TeamMember').values({ | |
| id: TeamMemberId.join(teamId, userId), | |
| teamId, | |
| userId, | |
| picture, | |
| preferredName, | |
| email, | |
| isLead: true, | |
| openDrawer: 'manageTeam' | |
| }) | |
| ) | |
| export default async function createTeamAndLeader( | |
| user: IUser, | |
| newTeam: ValidNewTeam, | |
| dataLoader: DataLoaderInstance | |
| ) { | |
| try { | |
| const r = await getRethink() | |
| const {id: userId, picture, preferredName, email} = user | |
| const {id: teamId, orgId} = newTeam | |
| const organization = await dataLoader.get('organizations').loadNonNull(orgId) | |
| const {tier, trialStartDate} = organization | |
| const verifiedTeam = new Team({...newTeam, createdBy: userId, tier, trialStartDate}) | |
| const meetingSettings = [ | |
| new MeetingSettingsRetrospective({teamId}), | |
| new MeetingSettingsAction({teamId}), | |
| new MeetingSettingsPoker({teamId}) | |
| ] | |
| const timelineEvent = new TimelineEventCreatedTeam({ | |
| createdAt: new Date(Date.now() + 5), | |
| userId, | |
| teamId, | |
| orgId | |
| }) | |
| const pg = getKysely() | |
| await Promise.all([ | |
| pg | |
| .with('TeamInsert', (qc) => qc.insertInto('Team').values(verifiedTeam)) | |
| .with('UserUpdate', (qc) => | |
| qc | |
| .updateTable('User') | |
| .set({tms: sql`arr_append_uniq("tms", ${teamId})`}) | |
| .where('id', '=', userId) | |
| ) | |
| .with('TeamMemberInsert', (qc) => | |
| qc.insertInto('TeamMember').values({ | |
| id: TeamMemberId.join(teamId, userId), | |
| teamId, | |
| userId, | |
| picture, | |
| preferredName, | |
| email, | |
| isLead: true, | |
| openDrawer: 'manageTeam' | |
| }) | |
| ) | |
| .insertInto('TimelineEvent') | |
| .values(timelineEvent) | |
| .execute(), | |
| // add meeting settings | |
| r.table('MeetingSettings').insert(meetingSettings).run(), | |
| // denormalize common fields to team member | |
| insertNewTeamMember(user, teamId, dataLoader) | |
| ]) | |
| dataLoader.clearAll(['teams', 'users', 'teamMembers', 'timelineEvents', 'meetingSettings']) | |
| } catch (error) { | |
| console.error('Error creating team and leader:', error) | |
| } | |
| } |
Comment on lines
51
to
57
| const orgTeams = (await dataLoader.get('teamsByOrgIds').loadMany(orgIds)).filter(isValid).flat() | ||
| const teamIds = orgTeams.map(({id}) => id) | ||
| const teamMembers = (await dataLoader.get('teamMembersByTeamId').loadMany(teamIds)) | ||
| .filter(isValid) | ||
| .flat() | ||
| const leadTeamMembers = teamMembers.filter(({isLead}) => isLead) | ||
| const leadUserIds = [...new Set(leadTeamMembers.map(({userId}) => userId))] |
There was a problem hiding this comment.
Add error handling for database operations.
The function should handle potential errors from the database operations to ensure robustness.
const requestToJoinDomain: MutationResolvers['requestToJoinDomain'] = async (
_source,
{},
{authToken, dataLoader}
) => {
try {
const r = await getRethink()
const operationId = dataLoader.share()
const subOptions = {operationId}
const pg = getKysely()
const viewerId = getUserId(authToken)
const viewer = await dataLoader.get('users').loadNonNull(viewerId)
const domain = getDomainFromEmail(viewer.email)
const now = new Date()
const orgIds = await getEligibleOrgIdsByDomain(domain, viewerId, dataLoader)
if (!orgIds.length) {
return standardError(new Error('No relevant organizations in the domain were found'))
}
const insertResult = await pg
.insertInto('DomainJoinRequest')
.values({
createdBy: viewerId,
domain,
expiresAt: new Date(now.getTime() + ms(`${REQUEST_EXPIRATION_DAYS}d`))
})
.onConflict((oc) => oc.columns(['createdBy', 'domain']).doNothing())
.returning('id')
.executeTakeFirst()
if (!insertResult) {
// Request is already exists, lets return success without sending duplicate notifications
return {success: true}
}
const orgTeams = (await dataLoader.get('teamsByOrgIds').loadMany(orgIds)).filter(isValid).flat()
const teamIds = orgTeams.map(({id}) => id)
const teamMembers = (await dataLoader.get('teamMembersByTeamId').loadMany(teamIds))
.filter(isValid)
.flat()
const leadTeamMembers = teamMembers.filter(({isLead}) => isLead)
const leadUserIds = [...new Set(leadTeamMembers.map(({userId}) => userId))]
const notificationsToInsert = leadUserIds.map((userId) => {
return new NotificationRequestToJoinOrg({
userId,
email: viewer.email,
name: viewer.preferredName,
picture: viewer.picture,
requestCreatedBy: viewerId,
domainJoinRequestId: insertResult.id
})
})
await r.table('Notification').insert(notificationsToInsert).run()
notificationsToInsert.forEach((notification) => {
publishNotification(notification, subOptions)
})
return {success: true}
} catch (error) {
console.error('Error requesting to join domain:', error)
return {success: false}
}
}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.
Suggested change
| const orgTeams = (await dataLoader.get('teamsByOrgIds').loadMany(orgIds)).filter(isValid).flat() | |
| const teamIds = orgTeams.map(({id}) => id) | |
| const teamMembers = (await dataLoader.get('teamMembersByTeamId').loadMany(teamIds)) | |
| .filter(isValid) | |
| .flat() | |
| const leadTeamMembers = teamMembers.filter(({isLead}) => isLead) | |
| const leadUserIds = [...new Set(leadTeamMembers.map(({userId}) => userId))] | |
| try { | |
| const orgTeams = (await dataLoader.get('teamsByOrgIds').loadMany(orgIds)).filter(isValid).flat() | |
| const teamIds = orgTeams.map(({id}) => id) | |
| const teamMembers = (await dataLoader.get('teamMembersByTeamId').loadMany(teamIds)) | |
| .filter(isValid) | |
| .flat() | |
| const leadTeamMembers = teamMembers.filter(({isLead}) => isLead) | |
| const leadUserIds = [...new Set(leadTeamMembers.map(({userId}) => userId))] | |
| } catch (error) { | |
| console.error('Error fetching team data:', error) | |
| return {success: false} | |
| } |
Signed-off-by: Matt Krick <matt.krick@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 9
Outside diff range, codebase verification and nitpick comments (2)
packages/server/postgres/migrations/1721356124871_TeamMember-phase2.ts (2)
1-4: Consider importing only necessary modules.Importing the entire
rethinkdb-tspackage might be unnecessary if only specific functions are used.- import {r} from 'rethinkdb-ts' + import {r, connect} from 'rethinkdb-ts'
54-54: Remove unnecessary console.log statement.The console.log statement is redundant and can be removed.
- await console.log('Adding index complete') + console.log('Adding index complete')
Comment on lines
+33
to
+41
| // some dates are | ||
| await r(dateDates) | ||
| .forEach((row: any) => { | ||
| return r | ||
| .table('TeamMember') | ||
| .get(row('id')) | ||
| .update({updatedAt: row('updatedAt')}) | ||
| }) | ||
| .run() |
There was a problem hiding this comment.
Handle potential errors during date conversion in RethinkDB.
Converting dates in RethinkDB should include error handling to manage potential issues.
- await r(dateDates)
- .forEach((row: any) => {
- return r
- .table('TeamMember')
- .get(row('id'))
- .update({updatedAt: row('updatedAt')})
- })
- .run()
+ try {
+ await r(dateDates)
+ .forEach((row: any) => {
+ return r
+ .table('TeamMember')
+ .get(row('id'))
+ .update({updatedAt: row('updatedAt')})
+ })
+ .run()
+ } catch (error) {
+ console.error('Failed to convert dates in RethinkDB:', error)
+ throw error
+ }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.
Suggested change
| // some dates are | |
| await r(dateDates) | |
| .forEach((row: any) => { | |
| return r | |
| .table('TeamMember') | |
| .get(row('id')) | |
| .update({updatedAt: row('updatedAt')}) | |
| }) | |
| .run() | |
| try { | |
| await r(dateDates) | |
| .forEach((row: any) => { | |
| return r | |
| .table('TeamMember') | |
| .get(row('id')) | |
| .update({updatedAt: row('updatedAt')}) | |
| }) | |
| .run() | |
| } catch (error) { | |
| console.error('Failed to convert dates in RethinkDB:', error) | |
| throw error | |
| } |
Comment on lines
+131
to
+137
| export async function down() { | ||
| await connectRethinkDB() | ||
| try { | ||
| await r.table('TeamMember').indexDrop('updatedAtId').run() | ||
| } catch { | ||
| // index already dropped | ||
| } |
There was a problem hiding this comment.
Avoid catching errors just to log and rethrow them.
Let the errors propagate naturally to be handled by the calling function or global error handler.
- try {
- await r.table('TeamMember').indexDrop('updatedAtId').run()
- } catch {
- // index already dropped
- }
+ await r.table('TeamMember').indexDrop('updatedAtId').run()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.
Suggested change
| export async function down() { | |
| await connectRethinkDB() | |
| try { | |
| await r.table('TeamMember').indexDrop('updatedAtId').run() | |
| } catch { | |
| // index already dropped | |
| } | |
| export async function down() { | |
| await connectRethinkDB() | |
| await r.table('TeamMember').indexDrop('updatedAtId').run() | |
| } |
Comment on lines
+8
to
+12
| const pg = new Kysely<any>({ | ||
| dialect: new PostgresDialect({ | ||
| pool: getPg() | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Ensure proper error handling for PostgreSQL connection.
The connection to PostgreSQL should include error handling to manage potential connection issues.
- const pg = new Kysely<any>({
- dialect: new PostgresDialect({
- pool: getPg()
- })
- })
+ let pg
+ try {
+ pg = new Kysely<any>({
+ dialect: new PostgresDialect({
+ pool: getPg()
+ })
+ })
+ } catch (error) {
+ console.error('Failed to connect to PostgreSQL:', error)
+ throw error
+ }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.
Suggested change
| const pg = new Kysely<any>({ | |
| dialect: new PostgresDialect({ | |
| pool: getPg() | |
| }) | |
| }) | |
| let pg | |
| try { | |
| pg = new Kysely<any>({ | |
| dialect: new PostgresDialect({ | |
| pool: getPg() | |
| }) | |
| }) | |
| } catch (error) { | |
| console.error('Failed to connect to PostgreSQL:', error) | |
| throw error | |
| } |
Comment on lines
+6
to
+7
| export async function up() { | ||
| await connectRethinkDB() |
There was a problem hiding this comment.
Ensure proper error handling for database connections.
The connection to RethinkDB should include error handling to manage potential connection issues.
- await connectRethinkDB()
+ try {
+ await connectRethinkDB()
+ } catch (error) {
+ console.error('Failed to connect to RethinkDB:', error)
+ throw error
+ }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.
Suggested change
| export async function up() { | |
| await connectRethinkDB() | |
| export async function up() { | |
| try { | |
| await connectRethinkDB() | |
| } catch (error) { | |
| console.error('Failed to connect to RethinkDB:', error) | |
| throw error | |
| } |
Comment on lines
+23
to
+27
| const strDates = await r | ||
| .table('TeamMember') | ||
| .filter((row) => row('updatedAt').typeOf().eq('STRING')) | ||
| .pluck('updatedAt', 'id', 'createdAt') | ||
| .run() |
There was a problem hiding this comment.
Handle potential errors during date filtering in RethinkDB.
Filtering dates in RethinkDB should include error handling to manage potential issues.
- const strDates = await r
- .table('TeamMember')
- .filter((row) => row('updatedAt').typeOf().eq('STRING'))
- .pluck('updatedAt', 'id', 'createdAt')
- .run()
+ let strDates
+ try {
+ strDates = await r
+ .table('TeamMember')
+ .filter((row) => row('updatedAt').typeOf().eq('STRING'))
+ .pluck('updatedAt', 'id', 'createdAt')
+ .run()
+ } catch (error) {
+ console.error('Failed to filter dates in RethinkDB:', error)
+ throw error
+ }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.
Suggested change
| const strDates = await r | |
| .table('TeamMember') | |
| .filter((row) => row('updatedAt').typeOf().eq('STRING')) | |
| .pluck('updatedAt', 'id', 'createdAt') | |
| .run() | |
| let strDates | |
| try { | |
| strDates = await r | |
| .table('TeamMember') | |
| .filter((row) => row('updatedAt').typeOf().eq('STRING')) | |
| .pluck('updatedAt', 'id', 'createdAt') | |
| .run() | |
| } catch (error) { | |
| console.error('Failed to filter dates in RethinkDB:', error) | |
| throw error | |
| } |
Comment on lines
+138
to
+143
| const pg = new Kysely<any>({ | ||
| dialect: new PostgresDialect({ | ||
| pool: getPg() | ||
| }) | ||
| }) | ||
| await pg.deleteFrom('TeamMember').execute() |
There was a problem hiding this comment.
Ensure proper error handling for PostgreSQL connection.
The connection to PostgreSQL should include error handling to manage potential connection issues.
- const pg = new Kysely<any>({
- dialect: new PostgresDialect({
- pool: getPg()
- })
- })
+ let pg
+ try {
+ pg = new Kysely<any>({
+ dialect: new PostgresDialect({
+ pool: getPg()
+ })
+ })
+ } catch (error) {
+ console.error('Failed to connect to PostgreSQL:', error)
+ throw error
+ }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.
Suggested change
| const pg = new Kysely<any>({ | |
| dialect: new PostgresDialect({ | |
| pool: getPg() | |
| }) | |
| }) | |
| await pg.deleteFrom('TeamMember').execute() | |
| let pg | |
| try { | |
| pg = new Kysely<any>({ | |
| dialect: new PostgresDialect({ | |
| pool: getPg() | |
| }) | |
| }) | |
| } catch (error) { | |
| console.error('Failed to connect to PostgreSQL:', error) | |
| throw error | |
| } | |
| await pg.deleteFrom('TeamMember').execute() |
Comment on lines
+43
to
+52
| try { | ||
| console.log('Adding index') | ||
| await r | ||
| .table('TeamMember') | ||
| .indexCreate('updatedAtId', (row: any) => [row('updatedAt'), row('id')]) | ||
| .run() | ||
| await r.table('TeamMember').indexWait().run() | ||
| } catch { | ||
| // index already exists | ||
| } |
There was a problem hiding this comment.
Avoid catching errors just to log and rethrow them.
Let the errors propagate naturally to be handled by the calling function or global error handler.
- try {
- console.log('Adding index')
- await r
- .table('TeamMember')
- .indexCreate('updatedAtId', (row: any) => [row('updatedAt'), row('id')])
- .run()
- await r.table('TeamMember').indexWait().run()
- } catch {
- // index already exists
- }
+ console.log('Adding index')
+ await r
+ .table('TeamMember')
+ .indexCreate('updatedAtId', (row: any) => [row('updatedAt'), row('id')])
+ .run()
+ await r.table('TeamMember').indexWait().run()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.
Suggested change
| try { | |
| console.log('Adding index') | |
| await r | |
| .table('TeamMember') | |
| .indexCreate('updatedAtId', (row: any) => [row('updatedAt'), row('id')]) | |
| .run() | |
| await r.table('TeamMember').indexWait().run() | |
| } catch { | |
| // index already exists | |
| } | |
| console.log('Adding index') | |
| await r | |
| .table('TeamMember') | |
| .indexCreate('updatedAtId', (row: any) => [row('updatedAt'), row('id')]) | |
| .run() | |
| await r.table('TeamMember').indexWait().run() |
Comment on lines
+14
to
+22
| // add a dummy date for nulls | ||
| const parabolEpoch = new Date('2016-06-01') | ||
| await r | ||
| .table('TeamMember') | ||
| .update((row) => ({ | ||
| updatedAt: row('updatedAt').default(parabolEpoch), | ||
| createdAt: row('createdAt').default(parabolEpoch) | ||
| })) | ||
| .run() |
There was a problem hiding this comment.
Handle potential errors during date updates in RethinkDB.
Updating dates in RethinkDB should include error handling to manage potential issues.
- await r
- .table('TeamMember')
- .update((row) => ({
- updatedAt: row('updatedAt').default(parabolEpoch),
- createdAt: row('createdAt').default(parabolEpoch)
- }))
- .run()
+ try {
+ await r
+ .table('TeamMember')
+ .update((row) => ({
+ updatedAt: row('updatedAt').default(parabolEpoch),
+ createdAt: row('createdAt').default(parabolEpoch)
+ }))
+ .run()
+ } catch (error) {
+ console.error('Failed to update dates in RethinkDB:', error)
+ throw error
+ }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.
Suggested change
| // add a dummy date for nulls | |
| const parabolEpoch = new Date('2016-06-01') | |
| await r | |
| .table('TeamMember') | |
| .update((row) => ({ | |
| updatedAt: row('updatedAt').default(parabolEpoch), | |
| createdAt: row('createdAt').default(parabolEpoch) | |
| })) | |
| .run() | |
| // add a dummy date for nulls | |
| const parabolEpoch = new Date('2016-06-01') | |
| try { | |
| await r | |
| .table('TeamMember') | |
| .update((row) => ({ | |
| updatedAt: row('updatedAt').default(parabolEpoch), | |
| createdAt: row('createdAt').default(parabolEpoch) | |
| })) | |
| .run() | |
| } catch (error) { | |
| console.error('Failed to update dates in RethinkDB:', error) | |
| throw error | |
| } |
Comment on lines
+75
to
+128
| let curUpdatedAt = r.minval | ||
| let curId = r.minval | ||
| for (let i = 0; i < 1e6; i++) { | ||
| console.log('inserting row', i * BATCH_SIZE, String(curUpdatedAt), String(curId)) | ||
| const rawRowsToInsert = (await r | ||
| .table('TeamMember') | ||
| .between([curUpdatedAt, curId], [r.maxval, r.maxval], { | ||
| index: 'updatedAtId', | ||
| leftBound: 'open', | ||
| rightBound: 'closed' | ||
| }) | ||
| .orderBy({index: 'updatedAtId'}) | ||
| .limit(BATCH_SIZE) | ||
| .pluck(...PG_COLS) | ||
| .run()) as TeamMember[] | ||
|
|
||
| const rowsToInsert = rawRowsToInsert.map((row) => { | ||
| const {preferredName, picture, ...rest} = row as any | ||
| return { | ||
| ...rest, | ||
| preferredName: preferredName.slice(0, 100), | ||
| picture: picture.slice(0, 2056) | ||
| } | ||
| }) | ||
| if (rowsToInsert.length === 0) break | ||
| const lastRow = rowsToInsert[rowsToInsert.length - 1] | ||
| curUpdatedAt = lastRow.updatedAt | ||
| curId = lastRow.id | ||
| try { | ||
| await pg | ||
| .insertInto('TeamMember') | ||
| .values(rowsToInsert) | ||
| .onConflict((oc) => oc.doNothing()) | ||
| .execute() | ||
| } catch (e) { | ||
| await Promise.all( | ||
| rowsToInsert.map(async (row) => { | ||
| try { | ||
| await pg | ||
| .insertInto('TeamMember') | ||
| .values(row) | ||
| .onConflict((oc) => oc.doNothing()) | ||
| .execute() | ||
| } catch (e) { | ||
| if (e.constraint === 'fk_userId' || e.constraint === 'fk_teamId') { | ||
| console.log(`Skipping ${row.id} because it has no user/team`) | ||
| return | ||
| } | ||
| console.log(e, row) | ||
| } | ||
| }) | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
Optimise data migration loop.
The data migration loop can be optimised by reducing the number of console.log statements and improving error handling.
- for (let i = 0; i < 1e6; i++) {
- console.log('inserting row', i * BATCH_SIZE, String(curUpdatedAt), String(curId))
+ for (let i = 0; ; i++) {
+ if (i % 100 === 0) {
+ console.log('inserting row', i * BATCH_SIZE, String(curUpdatedAt), String(curId))
+ }
const rawRowsToInsert = (await r
.table('TeamMember')
.between([curUpdatedAt, curId], [r.maxval, r.maxval], {
index: 'updatedAtId',
leftBound: 'open',
rightBound: 'closed'
})
.orderBy({index: 'updatedAtId'})
.limit(BATCH_SIZE)
.pluck(...PG_COLS)
.run()) as TeamMember[]
const rowsToInsert = rawRowsToInsert.map((row) => {
const {preferredName, picture, ...rest} = row as any
return {
...rest,
preferredName: preferredName.slice(0, 100),
picture: picture.slice(0, 2056)
}
})
if (rowsToInsert.length === 0) break
const lastRow = rowsToInsert[rowsToInsert.length - 1]
curUpdatedAt = lastRow.updatedAt
curId = lastRow.id
try {
await pg
.insertInto('TeamMember')
.values(rowsToInsert)
.onConflict((oc) => oc.doNothing())
.execute()
} catch (e) {
await Promise.all(
rowsToInsert.map(async (row) => {
try {
await pg
.insertInto('TeamMember')
.values(row)
.onConflict((oc) => oc.doNothing())
.execute()
} catch (e) {
if (e.constraint === 'fk_userId' || e.constraint === 'fk_teamId') {
console.log(`Skipping ${row.id} because it has no user/team`)
return
}
console.log(e, row)
}
})
)
}
}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.
Suggested change
| let curUpdatedAt = r.minval | |
| let curId = r.minval | |
| for (let i = 0; i < 1e6; i++) { | |
| console.log('inserting row', i * BATCH_SIZE, String(curUpdatedAt), String(curId)) | |
| const rawRowsToInsert = (await r | |
| .table('TeamMember') | |
| .between([curUpdatedAt, curId], [r.maxval, r.maxval], { | |
| index: 'updatedAtId', | |
| leftBound: 'open', | |
| rightBound: 'closed' | |
| }) | |
| .orderBy({index: 'updatedAtId'}) | |
| .limit(BATCH_SIZE) | |
| .pluck(...PG_COLS) | |
| .run()) as TeamMember[] | |
| const rowsToInsert = rawRowsToInsert.map((row) => { | |
| const {preferredName, picture, ...rest} = row as any | |
| return { | |
| ...rest, | |
| preferredName: preferredName.slice(0, 100), | |
| picture: picture.slice(0, 2056) | |
| } | |
| }) | |
| if (rowsToInsert.length === 0) break | |
| const lastRow = rowsToInsert[rowsToInsert.length - 1] | |
| curUpdatedAt = lastRow.updatedAt | |
| curId = lastRow.id | |
| try { | |
| await pg | |
| .insertInto('TeamMember') | |
| .values(rowsToInsert) | |
| .onConflict((oc) => oc.doNothing()) | |
| .execute() | |
| } catch (e) { | |
| await Promise.all( | |
| rowsToInsert.map(async (row) => { | |
| try { | |
| await pg | |
| .insertInto('TeamMember') | |
| .values(row) | |
| .onConflict((oc) => oc.doNothing()) | |
| .execute() | |
| } catch (e) { | |
| if (e.constraint === 'fk_userId' || e.constraint === 'fk_teamId') { | |
| console.log(`Skipping ${row.id} because it has no user/team`) | |
| return | |
| } | |
| console.log(e, row) | |
| } | |
| }) | |
| ) | |
| } | |
| } | |
| let curUpdatedAt = r.minval | |
| let curId = r.minval | |
| for (let i = 0; ; i++) { | |
| if (i % 100 === 0) { | |
| console.log('inserting row', i * BATCH_SIZE, String(curUpdatedAt), String(curId)) | |
| } | |
| const rawRowsToInsert = (await r | |
| .table('TeamMember') | |
| .between([curUpdatedAt, curId], [r.maxval, r.maxval], { | |
| index: 'updatedAtId', | |
| leftBound: 'open', | |
| rightBound: 'closed' | |
| }) | |
| .orderBy({index: 'updatedAtId'}) | |
| .limit(BATCH_SIZE) | |
| .pluck(...PG_COLS) | |
| .run()) as TeamMember[] | |
| const rowsToInsert = rawRowsToInsert.map((row) => { | |
| const {preferredName, picture, ...rest} = row as any | |
| return { | |
| ...rest, | |
| preferredName: preferredName.slice(0, 100), | |
| picture: picture.slice(0, 2056) | |
| } | |
| }) | |
| if (rowsToInsert.length === 0) break | |
| const lastRow = rowsToInsert[rowsToInsert.length - 1] | |
| curUpdatedAt = lastRow.updatedAt | |
| curId = lastRow.id | |
| try { | |
| await pg | |
| .insertInto('TeamMember') | |
| .values(rowsToInsert) | |
| .onConflict((oc) => oc.doNothing()) | |
| .execute() | |
| } catch (e) { | |
| await Promise.all( | |
| rowsToInsert.map(async (row) => { | |
| try { | |
| await pg | |
| .insertInto('TeamMember') | |
| .values(row) | |
| .onConflict((oc) => oc.doNothing()) | |
| .execute() | |
| } catch (e) { | |
| if (e.constraint === 'fk_userId' || e.constraint === 'fk_teamId') { | |
| console.log(`Skipping ${row.id} because it has no user/team`) | |
| return | |
| } | |
| console.log(e, row) | |
| } | |
| }) | |
| ) | |
| } | |
| } |
Signed-off-by: Matt Krick <matt.krick@gmail.com>
Signed-off-by: Matt Krick <matt.krick@gmail.com>
24 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Migrates existing R data to PG
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores