Interview: become unified and fully independent of core#1880
Open
JGStyle wants to merge 1 commit into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughAdds independent interview-review persistence and APIs, updates interview clients to load and save reviews separately from pass status, and resolves participation outputs for matching and interview-score display. ChangesInterview review backend
Interview review client state and editing
Resolved participation retrieval and matching display
Metadata and privacy integration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant InterviewDataShell
participant InterviewReviewAPI
participant InterviewReviewService
participant PostgreSQL
participant InterviewCard
InterviewDataShell->>InterviewReviewAPI: GET interview reviews
InterviewReviewAPI->>InterviewReviewService: load course-phase reviews
InterviewReviewService->>PostgreSQL: query interview_review
PostgreSQL-->>InterviewReviewService: review rows
InterviewReviewService-->>InterviewDataShell: InterviewReview[]
InterviewDataShell->>InterviewCard: store reviews
InterviewCard->>InterviewReviewAPI: PUT edited review
InterviewReviewAPI->>InterviewReviewService: upsert review
InterviewReviewService->>PostgreSQL: write interview_review
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
the
Migration
JGStyle
changed the title
Interview:Interview: become unified and fully independent of core
…reviewer) into db; adjust client side fetching
JGStyle
force-pushed
the
1705-interview-move-to-become-a-fully-independent-course-phase-microservice
branch
from
d2efb69 to
5d13796
Compare
Contributor
Author
|
rebased on main |
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
servers/interview/db/migration/0002_interview_review.up.sql (1)
6-6: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider a CHECK constraint on
scorefor defense in depth.
main.govalidates score range (1–5) at the application layer viaerrScoreOutOfRange, but a database-levelCHECK (score IS NULL OR (score >= 1 AND score <= 5))would guard against invalid data from direct SQL access or future code paths that bypass service validation.🛡️ Optional CHECK constraint
- score integer, + score integer CHECK (score IS NULL OR (score >= 1 AND score <= 5)),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@servers/interview/db/migration/0002_interview_review.up.sql` at line 6, Add a database-level CHECK constraint to the interview review table’s score column, allowing NULL values while restricting non-NULL scores to the inclusive range 1–5; update the migration in a reversible manner if applicable.servers/interview/interviewReview/router.go (1)
24-27: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winReturn generic error message for 500 responses.
handleErrorreturnserr.Error()directly to the client for all status codes. For 500 errors, service-layer messages wrap database errors (e.g.,"failed to upsert interview review: ERROR: duplicate key..."), exposing schema and constraint details. Return a generic message for server errors while keeping the detailed log.🔒 Suggested fix
func handleError(c *gin.Context, statusCode int, err error) { log.Error(err) + if statusCode >= 500 { + c.JSON(statusCode, gin.H{"error": "internal server error"}) + return + } c.JSON(statusCode, gin.H{"error": err.Error()}) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@servers/interview/interviewReview/router.go` around lines 24 - 27, Update handleError to continue logging the original err, but return a generic user-safe message such as “internal server error” when statusCode is 500 or another server-error status; preserve err.Error() for client-error responses.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@clients/interview_component/src/interview/network/hooks/useUpdateInterviewReview.ts`:
- Around line 10-19: Guard against an undefined coursePhaseID in
useUpdateInterviewReview before invoking updateInterviewReview; throw a clear
error instead of substituting an empty string, and ensure onSuccess only
invalidates the query using a valid coursePhaseID.
In `@clients/matching_component/src/matching/network/resolveParticipations.ts`:
- Around line 69-88: Add a finite per-request timeout to the axios.get call
within the resolutions mapping in resolveParticipations, using the request
configuration alongside headers. Preserve the existing error handling so
timed-out requests are logged and return an empty valuesByParticipation result.
In
`@clients/matching_component/src/matching/pages/MatchingParticipantsPage/MatchingParticipantsPage.tsx`:
- Around line 36-43: Update the extraData mapping in MatchingParticipantsPage so
stringValue is empty when prevData.score is either null or undefined, matching
the table’s N/A handling; only convert the score to String when it is non-null.
In `@servers/interview/interviewReview/interviewReviewDTO/interviewReview.go`:
- Around line 104-113: Update decodeInterviewAnswers to log a warning when
json.Unmarshal fails, including the unmarshal error and enough context to
identify the corrupted interview_answers data, before returning the empty slice.
---
Nitpick comments:
In `@servers/interview/db/migration/0002_interview_review.up.sql`:
- Line 6: Add a database-level CHECK constraint to the interview review table’s
score column, allowing NULL values while restricting non-NULL scores to the
inclusive range 1–5; update the migration in a reversible manner if applicable.
In `@servers/interview/interviewReview/router.go`:
- Around line 24-27: Update handleError to continue logging the original err,
but return a generic user-safe message such as “internal server error” when
statusCode is 500 or another server-error status; preserve err.Error() for
client-error responses.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1c8dd264-e8b7-4bfb-a08f-01695c49607f
📒 Files selected for processing (34)
clients/interview_component/src/interview/components/InterviewCard.tsxclients/interview_component/src/interview/components/StudentCard.tsxclients/interview_component/src/interview/hooks/useSorting.tsxclients/interview_component/src/interview/interfaces/InterviewReview.tsclients/interview_component/src/interview/network/hooks/useUpdateInterviewReview.tsclients/interview_component/src/interview/network/mutations/updateInterviewReview.tsclients/interview_component/src/interview/network/queries/getInterviewReviews.tsclients/interview_component/src/interview/pages/InterviewDataShell.tsxclients/interview_component/src/interview/zustand/useParticipationStore.tsxclients/matching_component/src/matching/MatchingOverviewPage.tsxclients/matching_component/src/matching/network/getResolvedCoursePhaseParticipations.tsclients/matching_component/src/matching/network/resolveParticipations.tsclients/matching_component/src/matching/pages/MatchingParticipantsPage/MatchingParticipantsPage.tsxservers/core/coursePhaseType/initializeTypes.goservers/core/db/migration/0026_interview_rest_provided_outputs.up.sqlservers/core/db/query/course_phase_participation.sqlservers/core/db/sqlc/course_phase_participation.sql.goservers/interview/database_dumps/base.sqlservers/interview/db/migration/0002_interview_review.up.sqlservers/interview/db/query/interview_review.sqlservers/interview/db/query/privacy.sqlservers/interview/db/sqlc/interview_review.sql.goservers/interview/db/sqlc/models.goservers/interview/db/sqlc/privacy.sql.goservers/interview/docs/docs.goservers/interview/docs/swagger.jsonservers/interview/docs/swagger.yamlservers/interview/interviewReview/interviewReviewDTO/interviewReview.goservers/interview/interviewReview/main.goservers/interview/interviewReview/router.goservers/interview/interviewReview/service.goservers/interview/interviewReview/service_test.goservers/interview/main.goservers/interview/privacy/service.go
Comment on lines
+10
to
+19
| export const useUpdateInterviewReview = (coursePhaseID: string | undefined) => { | ||
| const queryClient = useQueryClient() | ||
|
|
||
| return useMutation({ | ||
| mutationFn: ({ courseParticipationID, review }: UpdateInterviewReviewVariables) => | ||
| updateInterviewReview(coursePhaseID ?? '', courseParticipationID, review), | ||
| onSuccess: () => { | ||
| queryClient.invalidateQueries({ queryKey: ['interviewReviews', coursePhaseID] }) | ||
| }, | ||
| }) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Guard against undefined coursePhaseID in the mutation.
coursePhaseID ?? '' produces a malformed URL (interview/api/course_phase//interview-review/...) when coursePhaseID is undefined, and onSuccess invalidates ['interviewReviews', undefined] which won't match any active query. Consider throwing early when coursePhaseID is undefined so the caller gets a clear error instead of a silent 404.
💚 Suggested guard
export const useUpdateInterviewReview = (coursePhaseID: string | undefined) => {
const queryClient = useQueryClient()
return useMutation({
mutationFn: ({ courseParticipationID, review }: UpdateInterviewReviewVariables) => {
+ if (!coursePhaseID) {
+ throw new Error('coursePhaseID is required')
+ }
return updateInterviewReview(coursePhaseID, courseParticipationID, review)
},
onSuccess: () => {
queryClient.invalidateQueries({ queryKey: ['interviewReviews', coursePhaseID] })
},
})
}📝 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 const useUpdateInterviewReview = (coursePhaseID: string | undefined) => { | |
| const queryClient = useQueryClient() | |
| return useMutation({ | |
| mutationFn: ({ courseParticipationID, review }: UpdateInterviewReviewVariables) => | |
| updateInterviewReview(coursePhaseID ?? '', courseParticipationID, review), | |
| onSuccess: () => { | |
| queryClient.invalidateQueries({ queryKey: ['interviewReviews', coursePhaseID] }) | |
| }, | |
| }) | |
| export const useUpdateInterviewReview = (coursePhaseID: string | undefined) => { | |
| const queryClient = useQueryClient() | |
| return useMutation({ | |
| mutationFn: ({ courseParticipationID, review }: UpdateInterviewReviewVariables) => { | |
| if (!coursePhaseID) { | |
| throw new Error('coursePhaseID is required') | |
| } | |
| return updateInterviewReview(coursePhaseID, courseParticipationID, review) | |
| }, | |
| onSuccess: () => { | |
| queryClient.invalidateQueries({ queryKey: ['interviewReviews', coursePhaseID] }) | |
| }, | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@clients/interview_component/src/interview/network/hooks/useUpdateInterviewReview.ts`
around lines 10 - 19, Guard against an undefined coursePhaseID in
useUpdateInterviewReview before invoking updateInterviewReview; throw a clear
error instead of substituting an empty string, and ensure onSuccess only
invalidates the query using a valid coursePhaseID.
Comment on lines
+69
to
+88
| const resolvedDtos = await Promise.all( | ||
| resolutions.map(async (resolution): Promise<ResolvedDto> => { | ||
| const valuesByParticipation: Record<string, unknown> = {} | ||
| try { | ||
| const response = await axios.get<Array<Record<string, unknown>>>( | ||
| buildResolutionURL(resolution), | ||
| { headers: authHeaders() }, | ||
| ) | ||
| for (const item of response.data ?? []) { | ||
| const id = item.courseParticipationID | ||
| if (typeof id === 'string') { | ||
| valuesByParticipation[id] = item[resolution.dtoName] | ||
| } | ||
| } | ||
| } catch (err) { | ||
| console.error(`Failed to resolve "${resolution.dtoName}" from ${resolution.baseURL}`, err) | ||
| } | ||
| return { dtoName: resolution.dtoName, valuesByParticipation } | ||
| }), | ||
| ) |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Add a timeout to the axios requests to prevent indefinite hangs.
axios.get has no default timeout. If a provider endpoint is slow or unresponsive, Promise.all will block indefinitely, preventing the matching page from ever loading. The error handler catches network/HTTP errors but cannot catch a request that simply hangs.
⏱️ Proposed fix: add per-request timeout
const resolvedDtos = await Promise.all(
resolutions.map(async (resolution): Promise<ResolvedDto> => {
const valuesByParticipation: Record<string, unknown> = {}
try {
const response = await axios.get<Array<Record<string, unknown>>>(
buildResolutionURL(resolution),
- { headers: authHeaders() },
+ { headers: authHeaders(), timeout: 10_000 },
)📝 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 resolvedDtos = await Promise.all( | |
| resolutions.map(async (resolution): Promise<ResolvedDto> => { | |
| const valuesByParticipation: Record<string, unknown> = {} | |
| try { | |
| const response = await axios.get<Array<Record<string, unknown>>>( | |
| buildResolutionURL(resolution), | |
| { headers: authHeaders() }, | |
| ) | |
| for (const item of response.data ?? []) { | |
| const id = item.courseParticipationID | |
| if (typeof id === 'string') { | |
| valuesByParticipation[id] = item[resolution.dtoName] | |
| } | |
| } | |
| } catch (err) { | |
| console.error(`Failed to resolve "${resolution.dtoName}" from ${resolution.baseURL}`, err) | |
| } | |
| return { dtoName: resolution.dtoName, valuesByParticipation } | |
| }), | |
| ) | |
| const resolvedDtos = await Promise.all( | |
| resolutions.map(async (resolution): Promise<ResolvedDto> => { | |
| const valuesByParticipation: Record<string, unknown> = {} | |
| try { | |
| const response = await axios.get<Array<Record<string, unknown>>>( | |
| buildResolutionURL(resolution), | |
| { headers: authHeaders(), timeout: 10_000 }, | |
| ) | |
| for (const item of response.data ?? []) { | |
| const id = item.courseParticipationID | |
| if (typeof id === 'string') { | |
| valuesByParticipation[id] = item[resolution.dtoName] | |
| } | |
| } | |
| } catch (err) { | |
| console.error(`Failed to resolve "${resolution.dtoName}" from ${resolution.baseURL}`, err) | |
| } | |
| return { dtoName: resolution.dtoName, valuesByParticipation } | |
| }), | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@clients/matching_component/src/matching/network/resolveParticipations.ts`
around lines 69 - 88, Add a finite per-request timeout to the axios.get call
within the resolutions mapping in resolveParticipations, using the request
configuration alongside headers. Preserve the existing error handling so
timed-out requests are logged and return an empty valuesByParticipation result.
Comment on lines
+36
to
+43
| extraData: (participations ?? []).map((participation) => { | ||
| const score = participation.prevData?.score as number | undefined | ||
| return { | ||
| courseParticipationID: participation.courseParticipationID, | ||
| value: score ?? null, | ||
| stringValue: score !== undefined ? String(score) : '', | ||
| } | ||
| }), |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Export stringValue renders "null" for missing scores.
When prevData.score is null (not undefined), the guard score !== undefined passes and String(null) yields the literal "null" in the exported/extra data, whereas the table cell correctly shows N/A. Align the two so exports show an empty string for absent scores.
🔧 Proposed fix
extraData: (participations ?? []).map((participation) => {
const score = participation.prevData?.score as number | undefined
return {
courseParticipationID: participation.courseParticipationID,
value: score ?? null,
- stringValue: score !== undefined ? String(score) : '',
+ stringValue: score != null ? String(score) : '',
}
}),📝 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
| extraData: (participations ?? []).map((participation) => { | |
| const score = participation.prevData?.score as number | undefined | |
| return { | |
| courseParticipationID: participation.courseParticipationID, | |
| value: score ?? null, | |
| stringValue: score !== undefined ? String(score) : '', | |
| } | |
| }), | |
| extraData: (participations ?? []).map((participation) => { | |
| const score = participation.prevData?.score as number | undefined | |
| return { | |
| courseParticipationID: participation.courseParticipationID, | |
| value: score ?? null, | |
| stringValue: score != null ? String(score) : '', | |
| } | |
| }), |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@clients/matching_component/src/matching/pages/MatchingParticipantsPage/MatchingParticipantsPage.tsx`
around lines 36 - 43, Update the extraData mapping in MatchingParticipantsPage
so stringValue is empty when prevData.score is either null or undefined,
matching the table’s N/A handling; only convert the score to String when it is
non-null.
Comment on lines
+104
to
+113
| func decodeInterviewAnswers(raw []byte) []InterviewAnswer { | ||
| answers := make([]InterviewAnswer, 0) | ||
| if len(raw) == 0 { | ||
| return answers | ||
| } | ||
| if err := json.Unmarshal(raw, &answers); err != nil { | ||
| return make([]InterviewAnswer, 0) | ||
| } | ||
| return answers | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Log JSON unmarshal errors in decodeInterviewAnswers.
Corrupted interview_answers data is silently replaced with an empty slice, hiding data integrity issues. Add a log warning so corruption is diagnosable.
🛡️ Suggested fix
func decodeInterviewAnswers(raw []byte) []InterviewAnswer {
answers := make([]InterviewAnswer, 0)
if len(raw) == 0 {
return answers
}
if err := json.Unmarshal(raw, &answers); err != nil {
+ log.Warn("failed to decode interview answers: ", err)
return make([]InterviewAnswer, 0)
}
return answers
}📝 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
| func decodeInterviewAnswers(raw []byte) []InterviewAnswer { | |
| answers := make([]InterviewAnswer, 0) | |
| if len(raw) == 0 { | |
| return answers | |
| } | |
| if err := json.Unmarshal(raw, &answers); err != nil { | |
| return make([]InterviewAnswer, 0) | |
| } | |
| return answers | |
| } | |
| func decodeInterviewAnswers(raw []byte) []InterviewAnswer { | |
| answers := make([]InterviewAnswer, 0) | |
| if len(raw) == 0 { | |
| return answers | |
| } | |
| if err := json.Unmarshal(raw, &answers); err != nil { | |
| log.Warn("failed to decode interview answers: ", err) | |
| return make([]InterviewAnswer, 0) | |
| } | |
| return answers | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@servers/interview/interviewReview/interviewReviewDTO/interviewReview.go`
around lines 104 - 113, Update decodeInterviewAnswers to log a warning when
json.Unmarshal fails, including the unmarshal error and enough context to
identify the corrupted interview_answers data, before returning the empty slice.
5 tasks
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
✨ What is the change?
make interview cpm fully indepdent of core and unified:
at the same time, core now no longer has a special case for aggregating the participations data from the database for interview: same codebase for all now.
📌 Reason for the change / Link to issue
fixes #1705
🧪 How to Test
ensure the interview phase works exactly as expected before, including passing the score to the next phase
🖼️ Screenshots (if UI changes are included)
no UI changes, only work behind the scenes
✅ PR Checklist
Summary by CodeRabbit
New Features
Bug Fixes