[INTW26] Conflict Dialog#247
Conversation
4327539 to
a19d944
Compare
3877d7f to
cebb283
Compare
move the conflict pop-up into its own shared component and simplify the related review page code clean up the review table links and the shared GraphQL helper while keeping the current lockfile updates
Use applicant record IDs across review links, parsing, and submission requests so the review page matches the backend data model. This also keeps the current dialog typing change, login position field, and lockfile updates in the same commit
Revert the auth, GraphQL helper, and review list edits that were not part of the conflict dialog work. This keeps the branch focused on the dialog and applicant record ID wiring it depends on.
8f50c6f to
1109d38
Compare
mxc-maggiechen
left a comment
There was a problem hiding this comment.
Great work! I think this is good to go now 😄
ced2671 to
c9b5840
Compare
SaqAsh
left a comment
There was a problem hiding this comment.
Just a few questions, did review quickly on the train so feel free to point out anything I missed
a79f918 to
14a8bea
Compare
| ); | ||
| }; | ||
|
|
||
| type Props = ReviewStageProps & { header: ReactNode }; |
simplify the review dialog and helper code after PR feedback. remove placeholder interview footer actions and tighten the reviewer and applicant ID checks
| export const getReviewId = ( | ||
| query: Record<string, string | string[] | undefined>, | ||
| ): number => { | ||
| const reviewId = | ||
| typeof query["reviewId"] === "string" | ||
| ? parseInt(query["reviewId"]) | ||
| : (() => { | ||
| throw new Error("reviewId must be a String"); | ||
| })(); | ||
| if (Number.isNaN(reviewId)) | ||
| throw Error("reviewId must be parsable into an int"); |
There was a problem hiding this comment.
This is still kinda janky tbh, idk if there is a good pattern to parse URLs
There was a problem hiding this comment.
I think the cleanest way to do something like this is to use a query validator that is typed like zod
There was a problem hiding this comment.
apart from that tho it's fine
There was a problem hiding this comment.
oooh i kinda want to look in this and maybe some other native react options when we move to new repo. let's punt this for after migration?
| changeRating: ` | ||
| mutation changeRating($id: Int!, $ratingToBeChanged: String!, $newValue: Int!) { | ||
| mutation changeRating($id: String!, $ratingToBeChanged: String!, $newValue: Int!) { | ||
| changeRating(id: $id, ratingToBeChanged: $ratingToBeChanged, newValue: $newValue) { | ||
| id | ||
| } | ||
| } | ||
| `, | ||
| modifyFinalComments: ` | ||
| mutation modifyFinalComments($id: Int!, $newComments: String!, $newSkillCategory: String!, $newRecommendedSecondChoice: String!) { | ||
| mutation modifyFinalComments($id: String!, $newComments: String!, $newSkillCategory: String!, $newRecommendedSecondChoice: String!) { | ||
| modifyFinalComments(id: $id, newComments: $newComments, newSkillCategory: $newSkillCategory, newRecommendedSecondChoice: $newRecommendedSecondChoice) { |
There was a problem hiding this comment.
these changes are not really relevant tbh, because these endpoints are deprecated, changes are just made to avoid typing errors
There was a problem hiding this comment.
these changes are not really relevant tbh, because these endpoints are deprecated, changes are just made to avoid typing errors
| export const getReviewId = ( | ||
| query: Record<string, string | string[] | undefined>, | ||
| ): number => { | ||
| const reviewId = | ||
| typeof query["reviewId"] === "string" | ||
| ? parseInt(query["reviewId"]) | ||
| : (() => { | ||
| throw new Error("reviewId must be a String"); | ||
| })(); | ||
| if (Number.isNaN(reviewId)) | ||
| throw Error("reviewId must be parsable into an int"); |
There was a problem hiding this comment.
I think the cleanest way to do something like this is to use a query validator that is typed like zod
| export const getReviewId = ( | ||
| query: Record<string, string | string[] | undefined>, | ||
| ): number => { | ||
| const reviewId = | ||
| typeof query["reviewId"] === "string" | ||
| ? parseInt(query["reviewId"]) | ||
| : (() => { | ||
| throw new Error("reviewId must be a String"); | ||
| })(); | ||
| if (Number.isNaN(reviewId)) | ||
| throw Error("reviewId must be parsable into an int"); |
There was a problem hiding this comment.
apart from that tho it's fine
Notion ticket link
Implement Report Conflict Feature
Implementation description
Dialoguetemplate built on MUIDialog(components/common/Dialogue.tsx)components/review/stages/reviewInfoStage.tsx)reportReviewConflictGraphQL mutation ingraphql/queries.tsReviewPageAPIClientto call the mutation (APIClients/ReviewPageAPIClient.ts)Steps to test
Setup:
userIdreviewerIdyouruserId, note down theapplicantRecordId/review?applicantRecordId=<applicantRecordId>Conflictand verify the confirm dialog opensCanceland verify it closesYes, reportand verify the mutation is called and the success dialog opens (this will only work once backend pr is merged)What should reviewers focus on?