Ints26 reassign reviewer - #76
Conversation
| async reassignReviewer( | ||
| applicantRecordId: string, | ||
| oldReviewerId: string, | ||
| newReviewerId: string, | ||
| ): Promise<ReviewedApplicantRecordDTO> { | ||
| const transaction = await sequelize.transaction(); | ||
| try { | ||
| await this.deleteReviewedApplicantRecordByPk( | ||
| applicantRecordId, | ||
| oldReviewerId, | ||
| transaction, | ||
| ); | ||
| const newRecord = await this.createReviewedApplicantRecord( | ||
| { | ||
| applicantRecordId, | ||
| reviewerId: newReviewerId, | ||
| status: "TODO", | ||
| }, | ||
| transaction, | ||
| ); | ||
| await transaction.commit(); | ||
| return newRecord; | ||
| } catch (error: unknown) { | ||
| await transaction.rollback(); | ||
| Logger.error( | ||
| `Failed to reassign reviewer. Reason = ${getErrorMessage(error)}`, | ||
| ); | ||
| throw error; | ||
| } | ||
| } |
There was a problem hiding this comment.
this looks correct, let's not create a new service function though, we can just call these service functions in the GraphQL mutation: backend/typescript/graphql/resolvers/reviewedApplicantRecordResolvers.ts
I think the goal is to keep service functions as atomic to the table and reusable as possible. If we need joins from multiple tables, thats what our composite service files are for. But if we need to just call different service functions, we can just do it from the query/mutation.
| users.map(async (user) => { | ||
| let firebaseUser: firebaseAdmin.auth.UserRecord; | ||
| try { | ||
| firebaseUser = await firebaseAdmin.auth().getUser(user.auth_id); | ||
| } catch (error) { | ||
| Logger.error( | ||
| `user with authId ${user.auth_id} could not be fetched from Firebase`, | ||
| ); | ||
| throw error; | ||
| } | ||
|
|
There was a problem hiding this comment.
i dont think we need to fetch email from firebase, it should be recorded in users table, i think just your initial .findAll shoudl be fine. We also have a toUserDTO helper function that casts the sequelize type to our internal typescript DTO type
| boxShadow: "none", | ||
| opacity: 1, | ||
| width: "310px", | ||
| width: width, |
There was a problem hiding this comment.
nice, this is a lot better for a common dialgoue component 😄
There was a problem hiding this comment.
why did you need to edit this file? if its not necessary for this PR, lets revert it to keep this PR well scoped 😄
There was a problem hiding this comment.
remerged with main, but when a reviewer is reassigned i wanted to trigger dashboard to reload so added refetch. Not sure if there's another preferred way without needing to edit this file?
| const handleSortingChange = ( | ||
| updater: SortingState | ((old: SortingState) => SortingState), | ||
| ) => { |
There was a problem hiding this comment.
i don't think this change is within the scope of the PR either, lets revert it to keep this PR simple!
| const columns = useMemo( | ||
| () => | ||
| createReviewDashboardColumns({ | ||
| onReviewerClick: (row, reviewer) => { | ||
| setReassignmentTarget({ | ||
| applicantRecordId: row.applicantRecordId, | ||
| position: row.position, | ||
| reviewerId: reviewer.id, | ||
| reviewerName: `${reviewer.firstName} ${reviewer.lastName}`, | ||
| }); | ||
| }, | ||
| }), | ||
| [], | ||
| ); |
There was a problem hiding this comment.
i think there might be a cleaner way to add a callback per cell? i haven't used tanstack before either, i just asked cursor for a bit and it cooked up this: #85
let me know your thoughts! if you think its good, we can merge that change into your branch
2 participants
Notion ticket link
Reassign Reviewer on Dashboard Feature
Implementation description
getUsersByPositionquery to fetch all users with the same position as the applicant record, used to populate the reassign reviewer dropdownreassignReviewermutation that atomically deletes the existingreviewedApplicantRecordfor the old reviewer and creates a new one for the new reviewer using a Sequelize transactioncreateReviewedApplicantRecordanddeleteReviewedApplicantRecordByPkservice functions to accept an optional Sequelize transaction parametergetUsersByPositionandreassignReviewertoReviewPageAPIClientSteps to test
What should reviewers focus on?
reassignReviewerensure if either the delete or create fails, both operations are rolled back correctlycreateReviewedApplicantRecordanddeleteReviewedApplicantRecordByPkdoesn't break existing callersChecklist