Intro Course Developer Profile Instructor View
#256
Conversation
WalkthroughThe pull request introduces a new route and sidebar link for accessing developer profiles. It updates the developer profile types to include additional properties and introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Frontend UI
participant Client as API Function (updateDeveloperProfile)
participant Server as Server Endpoint
participant Service as DeveloperProfile Service
participant DB as Database
UI->>Client: Submit updated developer profile
Client->>Server: PUT /developerProfile/:courseParticipationID
Server->>Service: Call CreateOrUpdateDeveloperProfile(ctx, phaseID, participationID, profile)
Service->>DB: Execute upsert SQL command
DB-->>Service: Return confirmation
Service-->>Server: Return response
Server-->>Client: 200 OK / Error
Client-->>UI: Update UI based on response
Poem
✨ Finishing Touches
🪧 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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Jetzt auch mit Validation für dich @Mtze |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (14)
clients/intro_course_developer_component/src/introCourse/network/mutations/updateDeveloperProfile.ts (1)
19-22: Consider improving error handling.The current error handling logs the error and then rethrows it, which might lead to duplicate error logging if the caller also logs the error.
Consider removing the console logging and let the caller handle errors:
} catch (err) { - console.error(err) throw err }Alternatively, add more context to the error for better debugging:
} catch (err) { - console.error(err) + console.error(`Failed to update developer profile: ${err.message}`, err) throw err }clients/intro_course_developer_component/src/introCourse/pages/DeveloperProfilesLecturer/hooks/useGetSortedParticipations.tsx (1)
1-40: Well-structured custom hook for sortingThe implementation of
useGetSortedParticipationsis clean and follows React best practices with proper use ofuseMemofor optimization.However, I'd recommend adding input validation to handle edge cases:
export const useGetSortedParticipations = ( sortConfig: | { key: string direction: 'ascending' | 'descending' } | undefined, participantsWithProfiles: { participation: CoursePhaseParticipationWithStudent profile: DeveloperProfile | undefined }[], ) => { return useMemo(() => { + if (!participantsWithProfiles || participantsWithProfiles.length === 0) { + return [] + } const sorted = [...participantsWithProfiles] if (!sortConfig) return sorted return sorted.sort((a, b) => { let aValue: string | number = '' let bValue: string | number = '' if (sortConfig.key === 'name') { aValue = `${a.participation.student.firstName} ${a.participation.student.lastName}`.toLowerCase() bValue = `${b.participation.student.firstName} ${b.participation.student.lastName}`.toLowerCase() } else if (sortConfig.key === 'profileStatus') { aValue = a.profile ? 1 : 0 bValue = b.profile ? 1 : 0 } if (aValue < bValue) return sortConfig.direction === 'ascending' ? -1 : 1 if (aValue > bValue) return sortConfig.direction === 'ascending' ? 1 : -1 return 0 }) }, [participantsWithProfiles, sortConfig]) }clients/intro_course_developer_component/src/introCourse/pages/DeveloperProfilesLecturer/hooks/useGetParticipationsWithProfiles.tsx (1)
5-22: Consider adding explicit return type and removing unnecessary optional chaining.The hook correctly maps participants to their profiles, but could benefit from two improvements:
- Add an explicit return type for better type safety
- Remove unnecessary optional chaining on developerProfiles (line 13) as it's already a parameter
- Remove the unnecessary fallback empty array (line 19) as the mapping would already handle an empty array
export const useGetParticipationsWithProfiles = ( participants: CoursePhaseParticipationWithStudent[], developerProfiles: DeveloperProfile[], +) : { participation: CoursePhaseParticipationWithStudent, profile: DeveloperProfile | undefined }[] => { return useMemo(() => { return ( participants.map((participation) => { const profile = - developerProfiles?.find( + developerProfiles.find( (devProfile) => devProfile.courseParticipationID === participation.courseParticipationID, ) || undefined return { participation, profile } - }) || [] + }) ) }, [participants, developerProfiles]) }clients/intro_course_developer_component/src/introCourse/pages/DeveloperProfilesLecturer/components/FilterMenu.tsx (3)
29-50: Implement consistent event handling throughout the component.The survey status filter items use
onCheckedChangewhile device filters use customonClickhandlers withe.preventDefault(). This inconsistency should be addressed.Consider using the same approach for all filter items for consistency:
<DropdownMenuCheckboxItem checked={filters.devices.macBook} - onClick={(e) => { - e.preventDefault() + onCheckedChange={(checked) => setFilters((prevFilters) => ({ ...filters, devices: { ...prevFilters.devices, noDevices: false, - macBook: !prevFilters.devices.macBook, + macBook: checked, }, })) - }} + } >
57-67: Fix potential stale state reference in the state update.When updating state with the previous state, consistently use the previous state reference in all spread operations.
setFilters((prevFilters) => ({ - ...filters, + ...prevFilters, devices: { ...prevFilters.devices, noDevices: false, macBook: !prevFilters.devices.macBook, }, }))
124-140: Consider extracting "No Devices" logic to a separate function.The reset logic for the "No Devices" option could be extracted to improve readability.
Consider extracting this logic:
const handleNoDevicesChange = (e: React.MouseEvent) => { e.preventDefault() setFilters((prevFilters) => ({ ...prevFilters, devices: { noDevices: !prevFilters.devices.noDevices, macBook: false, iPhone: false, iPad: false, appleWatch: false, }, })) }clients/intro_course_developer_component/src/introCourse/pages/DeveloperProfilesLecturer/DeveloperProfilesLecturerPage.tsx (4)
41-47: Avoid usinganytype - replace with specific type for better type safety.The
anytype forparticipationshould be replaced with a more specific type likeCoursePhaseParticipationWithStudent.const [selectedParticipant, setSelectedParticipant] = useState< | { - participation: any + participation: CoursePhaseParticipationWithStudent profile: DeveloperProfile | undefined } | undefined >(undefined)
150-166: Enhance accessibility for sortable table headers.Add proper ARIA attributes to indicate that these table headers are sortable to improve accessibility.
<TableHead className='cursor-pointer' onClick={() => requestSort('name')} + aria-sort={sortConfig?.key === 'name' ? sortConfig.direction : 'none'} + role="columnheader" + aria-label="Sort by name" >Also applies to: 167-186
98-101: Consider enhancing the refresh function to handle errors.The
handleRefreshfunction refetches both data sources but doesn't handle potential errors during refetching.const handleRefresh = () => { - refetchCoursePhaseParticipations() - refetchDeveloperProfiles() + Promise.all([ + refetchCoursePhaseParticipations(), + refetchDeveloperProfiles() + ]).catch((error) => { + console.error('Error refreshing data:', error) + // Consider showing a toast or notification to the user + }) }
1-267: Consider breaking down this large component into smaller components.This page component is quite large (267 lines) and handles multiple concerns including state management, API calls, filtering, sorting, and rendering. Consider breaking it down into smaller, more focused components or custom hooks for better maintainability.
You could extract:
- A
ParticipantTablecomponent for the table rendering- A
ParticipantRowcomponent for each row- A
useDeveloperProfilesDatahook for data fetching logicThis would make the main component more focused on composition and higher-level concerns.
clients/intro_course_developer_component/src/introCourse/pages/DeveloperProfilesLecturer/hooks/useGetFilteredParticipations.tsx (2)
13-29: Consider refactoring the survey status filter logicThe survey status filter logic works correctly but could be simplified for better readability.
- const surveyFilterActive = filters.surveyStatus.completed || filters.surveyStatus.notCompleted - let passesSurvey = true - if (surveyFilterActive) { - // Assume false initially then try matching any active filter. - passesSurvey = false - if (filters.surveyStatus.completed && profile) { - passesSurvey = true - } - if (filters.surveyStatus.notCompleted && !profile) { - passesSurvey = true - } - } + const surveyFilterActive = filters.surveyStatus.completed || filters.surveyStatus.notCompleted + const passesSurvey = !surveyFilterActive || + (filters.surveyStatus.completed && profile) || + (filters.surveyStatus.notCompleted && !profile)
34-45: Use optional chaining for safer property accessThe current implementation uses a verbose pattern to check if profile exists before accessing its properties. Using optional chaining would make the code more concise and readable.
The static analysis tool correctly identified this issue. Apply this change to all device checks:
- passesDevices = passesDevices && !!(profile && profile.hasMacBook) + passesDevices = passesDevices && !!profile?.hasMacBook - passesDevices = passesDevices && !!(profile && profile.iPhoneUUID) + passesDevices = passesDevices && !!profile?.iPhoneUUID - passesDevices = passesDevices && !!(profile && profile.iPadUUID) + passesDevices = passesDevices && !!profile?.iPadUUID - passesDevices = passesDevices && !!(profile && profile.appleWatchUUID) + passesDevices = passesDevices && !!profile?.appleWatchUUID🧰 Tools
🪛 Biome (1.9.4)
[error] 35-35: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 38-38: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 41-41: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 44-44: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
servers/intro_course/db/sqlc/developerProfile.sql.go (1)
67-76: Consider consolidating duplicate parameter structsThe
CreateOrUpdateDeveloperProfileParamsstruct is identical to the existingCreateDeveloperProfileParamsstruct. While this is auto-generated code, it might be worth considering a refactoring of the SQL queries to use a common parameter struct if possible.servers/intro_course/developerProfile/router.go (1)
31-32: Fix typo in commentThere's a small typo in the comment.
- // ger course participation id from context + // get course participation id from context
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
clients/intro_course_developer_component/routes/index.tsx(2 hunks)clients/intro_course_developer_component/sidebar/index.tsx(1 hunks)clients/intro_course_developer_component/src/introCourse/interfaces/DeveloperProfile.tsx(1 hunks)clients/intro_course_developer_component/src/introCourse/interfaces/PostDeveloperProfile.tsx(1 hunks)clients/intro_course_developer_component/src/introCourse/network/mutations/postDeveloperProfile.ts(1 hunks)clients/intro_course_developer_component/src/introCourse/network/mutations/updateDeveloperProfile.ts(1 hunks)clients/intro_course_developer_component/src/introCourse/network/queries/getAllDeveloperProfiles.ts(1 hunks)clients/intro_course_developer_component/src/introCourse/pages/DeveloperProfile/DeveloperProfileForm.tsx(3 hunks)clients/intro_course_developer_component/src/introCourse/pages/DeveloperProfile/DeveloperProfilePage.tsx(3 hunks)clients/intro_course_developer_component/src/introCourse/pages/DeveloperProfilesLecturer/DeveloperProfilesLecturerPage.tsx(1 hunks)clients/intro_course_developer_component/src/introCourse/pages/DeveloperProfilesLecturer/components/FilterMenu.tsx(1 hunks)clients/intro_course_developer_component/src/introCourse/pages/DeveloperProfilesLecturer/components/ProfileDetailsDialog.tsx(1 hunks)clients/intro_course_developer_component/src/introCourse/pages/DeveloperProfilesLecturer/hooks/useGetFilteredParticipations.tsx(1 hunks)clients/intro_course_developer_component/src/introCourse/pages/DeveloperProfilesLecturer/hooks/useGetParticipationsWithProfiles.tsx(1 hunks)clients/intro_course_developer_component/src/introCourse/pages/DeveloperProfilesLecturer/hooks/useGetSortedParticipations.tsx(1 hunks)clients/intro_course_developer_component/src/introCourse/pages/DeveloperProfilesLecturer/interfaces/devProfileFilter.ts(1 hunks)clients/intro_course_developer_component/src/introCourse/validations/instructorDevProfile.ts(1 hunks)servers/intro_course/db/query/developerProfile.sql(1 hunks)servers/intro_course/db/sqlc/developerProfile.sql.go(1 hunks)servers/intro_course/developerProfile/developerProfileDTO/get_developer_profile.go(2 hunks)servers/intro_course/developerProfile/developerProfileDTO/post_developer_profile.go(2 hunks)servers/intro_course/developerProfile/router.go(2 hunks)servers/intro_course/developerProfile/service.go(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
clients/intro_course_developer_component/src/introCourse/pages/DeveloperProfilesLecturer/hooks/useGetFilteredParticipations.tsx
[error] 35-35: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 38-38: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 41-41: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 44-44: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: build-servers / core / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-server-core
- GitHub Check: build-servers / intro-course / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-server-intro-course
- GitHub Check: build-clients / clients-base / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-clients-base
- GitHub Check: build-servers / core / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-server-core
- GitHub Check: build-clients / clients-base / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-clients-base
- GitHub Check: build-servers / intro-course / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-server-intro-course
- GitHub Check: Mend Security Check
🔇 Additional comments (34)
clients/intro_course_developer_component/src/introCourse/interfaces/DeveloperProfile.tsx (1)
1-10: New properties added align well with the PR objectivesThe addition of
coursePhaseIDandcourseParticipationIDto theDeveloperProfiletype is appropriate for the Lecturer View implementation. These fields will allow for proper association of developer profiles with specific course phases and student participations.clients/intro_course_developer_component/src/introCourse/network/queries/getAllDeveloperProfiles.ts (1)
4-17: Query function implementation looks goodThe implementation of
getAllDeveloperProfilesfollows a clean pattern with proper error handling. It correctly uses theintroCourseAxiosInstancefor consistency with other API calls in the project.clients/intro_course_developer_component/src/introCourse/interfaces/PostDeveloperProfile.tsx (1)
1-8: Clean separation of concerns with PostDeveloperProfile typeCreating a separate
PostDeveloperProfiletype that excludes server-side identifiers likecoursePhaseIDandcourseParticipationIDis a good design decision. This maintains a clear distinction between the data model used for displaying profiles and the one used for submitting updates.clients/intro_course_developer_component/src/introCourse/pages/DeveloperProfilesLecturer/interfaces/devProfileFilter.ts (1)
1-13: Well-designed filter structure.This type definition provides a clear interface for filtering developer profiles by survey status and device ownership. The boolean flags make it easy to toggle filters in the UI.
servers/intro_course/db/query/developerProfile.sql (1)
17-36: Good implementation of SQL upsert operation.The SQL command is well-structured with proper conflict handling on the composite key
(course_phase_id, course_participation_id). This approach efficiently handles both creation and updates in a single database operation.clients/intro_course_developer_component/src/introCourse/network/mutations/updateDeveloperProfile.ts (1)
4-23: Consider return type discrepancy and content type.Two issues to consider:
- The function has a return type of
Promise<void>but returns the axios response object, which would bePromise<AxiosResponse<any>>.- The Content-Type header uses
application/json-path+jsonwhich is uncommon - standard REST APIs typically useapplication/json.Does the backend specifically require
application/json-path+jsoncontent type, or wouldapplication/jsonwork? Also, confirm whether you need to return data from this API call or ifvoidis the correct return type.export const updateDeveloperProfile = async ( coursePhaseID: string, courseParticipationID: string, developerProfile: PostDeveloperProfile, -): Promise<void> => { +): Promise<any> => { try { return await introCourseAxiosInstance.put( `intro-course/api/course_phase/${coursePhaseID}/developer_profile/${courseParticipationID}`, developerProfile, { headers: { - 'Content-Type': 'application/json-path+json', + 'Content-Type': 'application/json', }, }, ) } catch (err) { console.error(err) throw err } }servers/intro_course/developerProfile/service.go (1)
81-96: Implementation follows established pattern for data operationsThe new
CreateOrUpdateDeveloperProfilefunction follows the same pattern as other functions in this service, properly handling context, error logging, and return values. It correctly implements an upsert operation that can both create new developer profiles and update existing ones.One observation: this new function accepts a
DeveloperProfiletype while the existingCreateDeveloperProfilefunction usesPostDeveloperProfile. This distinction suggests they serve different purposes in the API design.clients/intro_course_developer_component/src/introCourse/validations/instructorDevProfile.ts (1)
1-22: Good validation implementation with proper type handlingThe validation schema correctly defines required fields and appropriate validations:
- Email validation for
appleID- Required fields for core properties
- Smart handling of optional UUID fields with preprocessing of empty strings
This approach will provide clear error messages during form validation while maintaining type safety throughout the application.
clients/intro_course_developer_component/src/introCourse/network/mutations/postDeveloperProfile.ts (1)
1-1: Type update aligns with data model changesUpdating the import and parameter type from
DeveloperProfiletoPostDeveloperProfileensures consistency with the updated data model. This change supports the new instructor view functionality described in the PR objectives.Also applies to: 6-6
servers/intro_course/developerProfile/developerProfileDTO/post_developer_profile.go (2)
12-12: Fixed JSON field casing consistencyGood correction to the JSON tag to ensure consistent casing between the backend and frontend (
hasMacBookinstead ofhasMacbook). This avoids potential data mapping issues.
31-42: Well-structured DTO mapping for upsert operationThe new
GetDeveloperProfileDTOFromCreateRequestfunction properly maps theDeveloperProfilefields to the database parameters required for the upsert operation. The implementation correctly handles all the necessary fields and maintains consistency with the existing pattern.clients/intro_course_developer_component/src/introCourse/pages/DeveloperProfile/DeveloperProfilePage.tsx (3)
9-9: Import updated to use PostDeveloperProfile typeThis change correctly updates the import to use the new
PostDeveloperProfiletype instead of the previousDeveloperProfiletype, aligning with the revised data model for submitting developer profiles.
24-25: Method signature updated for mutation functionThe mutation function now correctly expects a
PostDeveloperProfiletype rather thanDeveloperProfile, ensuring type consistency throughout the submission flow.
42-42: Simplified submission flowThe code now directly calls
mutation.mutate(profile)instead of using an intermediary function, reducing unnecessary abstraction and making the code more maintainable.clients/intro_course_developer_component/src/introCourse/pages/DeveloperProfile/DeveloperProfileForm.tsx (4)
3-3: Import updated to use PostDeveloperProfile typeThis change correctly updates the import statement to use the new
PostDeveloperProfiletype.
21-21: Appropriate import of DeveloperProfile typeAdding the import for
DeveloperProfileis necessary since it's still used in the component props for displaying existing profiles.
26-26: Updated prop signature to use PostDeveloperProfileThe
onSubmitprop type is correctly updated to usePostDeveloperProfileinstead ofDeveloperProfile, maintaining type consistency with the parent component.
59-59: Updated variable type to PostDeveloperProfileThe
submittedProfilevariable is now correctly typed asPostDeveloperProfile, ensuring type consistency throughout the submission flow.servers/intro_course/developerProfile/developerProfileDTO/get_developer_profile.go (1)
14-14: Fixed JSON tag casing consistencyThe JSON tag has been corrected from
hasMacbooktohasMacBookto ensure consistent casing in the JSON representation.clients/intro_course_developer_component/src/introCourse/pages/DeveloperProfilesLecturer/components/ProfileDetailsDialog.tsx (4)
43-81: Good implementation of the dialog form with error handling.The React Hook Form setup with Zod validation is well-implemented, and the error handling in the mutation is thorough with proper error message extraction and display.
83-85: Form submission handler looks good.The submission handler correctly passes the form data to the mutation function.
100-104: Well-implemented error display.The error message display is properly implemented with appropriate styling.
106-225: Form structure is well-organized with appropriate UI components.The form layout is clean with good organization of fields into logical sections. The device section with icons enhances usability.
clients/intro_course_developer_component/src/introCourse/pages/DeveloperProfilesLecturer/DeveloperProfilesLecturerPage.tsx (2)
128-136: Good implementation of the sorting function.The sorting logic is well-implemented with appropriate toggling of the sorting direction.
139-264: Well-structured table with clear display of profile information.The table layout is clean with good organization of data. The filtered count information and the device icons enhance usability. The dialog integration is also well implemented.
clients/intro_course_developer_component/src/introCourse/pages/DeveloperProfilesLecturer/hooks/useGetFilteredParticipations.tsx (4)
1-5: Clean import structureThe imports are well-organized and appropriately typed, making dependencies clear.
6-12: Well-typed hook parametersThe hook is properly typed with explicit interface definitions, which improves type safety and code readability.
46-55: Clear implementation of "No Devices" filter logicThe implementation correctly handles the "No Devices" filter by checking that the profile either doesn't exist or doesn't have any of the devices. The comment clearly explains the logic.
57-60: Efficient memoization with appropriate dependenciesThe hook correctly uses
useMemowith the proper dependencies array, ensuring the filtered results are only recalculated when necessary.servers/intro_course/db/sqlc/developerProfile.sql.go (2)
45-65: Well-structured SQL upsert queryThe
createOrUpdateDeveloperProfileSQL query is well-formatted and properly implements the upsert pattern using theON CONFLICTclause. The consistent indentation and column alignment make it easy to read.
78-90: Implementation follows established patterns consistentlyThe
CreateOrUpdateDeveloperProfilefunction follows the same implementation pattern as other functions in the file, maintaining consistency in the codebase.servers/intro_course/developerProfile/router.go (3)
19-20: New route follows authentication and authorization patternsThe implementation of the PUT endpoint correctly follows the pattern established by other routes, requiring appropriate authentication through the middleware.
92-119: Comprehensive error handling in update endpointThe
updateDeveloperProfilefunction follows good practices by:
- Properly parsing and validating UUID parameters
- Providing descriptive error logs
- Using appropriate HTTP status codes for different error conditions
- Following the same error handling pattern as other endpoints
113-113:✅ Verification successful
Verify CreateOrUpdateDeveloperProfile implementation
Make sure that the
CreateOrUpdateDeveloperProfilefunction is properly implemented in a separate file, as it's being called here but not defined within this file.
🏁 Script executed:
#!/bin/bash # Check for implementation of CreateOrUpdateDeveloperProfile function grep -r "func CreateOrUpdateDeveloperProfile" --include="*.go" servers/intro_course/Length of output: 304
Implementation Verified
TheCreateOrUpdateDeveloperProfilefunction has been confirmed to be implemented in the separate fileservers/intro_course/developerProfile/service.go. No further changes are required based on this review.
| { | ||
| title: 'Developer Profiles', | ||
| goToPath: '/developer-profiles', | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Missing required permissions for Developer Profiles sidebar item
Unlike other sidebar items like "Tutor Import" and "Settings" which have requiredPermissions specified, the new "Developer Profiles" item lacks permission restrictions. Consider whether this is intentional, as the PR description mentions this is a "Lecturer View" which suggests it should have similar permission restrictions to the other lecturer-specific items.
{
title: 'Developer Profiles',
goToPath: '/developer-profiles',
+ requiredPermissions: [Role.PROMPT_ADMIN, Role.COURSE_LECTURER],
},📝 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.
| { | |
| title: 'Developer Profiles', | |
| goToPath: '/developer-profiles', | |
| }, | |
| { | |
| title: 'Developer Profiles', | |
| goToPath: '/developer-profiles', | |
| requiredPermissions: [Role.PROMPT_ADMIN, Role.COURSE_LECTURER], | |
| }, |
| { | ||
| path: '/developer-profiles', | ||
| element: <DeveloperProfilesLecturerPage />, | ||
| }, |
There was a problem hiding this comment.
Security concern: Missing required permissions for developer profiles route.
Unlike other routes in this file, the new developer profiles route doesn't specify requiredPermissions. Since this is labeled as a "Lecturer View" in the PR description, it should likely have the same permission requirements as other lecturer-only routes.
{
path: '/developer-profiles',
element: <DeveloperProfilesLecturerPage />,
+ requiredPermissions: [Role.PROMPT_ADMIN, Role.COURSE_LECTURER],
},📝 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.
| { | |
| path: '/developer-profiles', | |
| element: <DeveloperProfilesLecturerPage />, | |
| }, | |
| { | |
| path: '/developer-profiles', | |
| element: <DeveloperProfilesLecturerPage />, | |
| requiredPermissions: [Role.PROMPT_ADMIN, Role.COURSE_LECTURER], | |
| }, |
servers/intro_course/developerProfile/developerProfileDTO/get_developer_profile.go
Show resolved
Hide resolved
servers/intro_course/developerProfile/developerProfileDTO/get_developer_profile.go
Show resolved
Hide resolved
niclasheun
requested a deployment
to
prompt-dev-vm
GitHub Actions
Waiting
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
servers/intro_course/developerProfile/router.go (3)
92-119: Implementation follows established patterns but lacks documentationThe
updateDeveloperProfilefunction correctly handles parameter parsing, error checking, and delegates to the service layer, following the same pattern as other handlers in this file.Consider adding documentation comments for this function to maintain consistency with the codebase style.
+// updateDeveloperProfile handles PUT requests to update an existing developer profile +// This endpoint is restricted to users with PromptAdmin or CourseLecturer roles func updateDeveloperProfile(c *gin.Context) {
107-111: Consider adding validation for the developer profile dataWhile the function correctly binds JSON to the
devProfilestruct, there's no validation of the data beyond ensuring it can be parsed from JSON. Consider adding validation to ensure the data meets your business requirements before updating the database.var devProfile developerProfileDTO.DeveloperProfile if err := c.BindJSON(&devProfile); err != nil { handleError(c, http.StatusBadRequest, err) return } + +// Validate required fields +if err := validateDeveloperProfile(devProfile); err != nil { + handleError(c, http.StatusBadRequest, err) + return +}
113-118: Consider returning the updated profile in the responseOther endpoints like
getOwnDeveloperProfilereturn the profile data in the response, but this endpoint only returns a status code. For consistency and to help clients avoid an additional API call, consider returning the updated profile in the response.err = CreateOrUpdateDeveloperProfile(c, coursePhaseID, courseParticipationID, devProfile) if err != nil { handleError(c, http.StatusInternalServerError, err) return } -c.Status(http.StatusOK) + +// Return the updated profile +updatedProfile, err := GetDeveloperProfile(c, coursePhaseID, courseParticipationID) +if err != nil { + handleError(c, http.StatusInternalServerError, err) + return +} +c.JSON(http.StatusOK, updatedProfile)clients/intro_course_developer_component/src/introCourse/pages/DeveloperProfilesLecturer/DeveloperProfilesLecturerPage.tsx (3)
30-41: Keep naming consistent in custom hooks.
Your naming structure (useGetParticipationsWithProfiles,useGetSortedParticipations,useGetFilteredParticipations) is clear. Ensure that you maintain consistency and a predictable naming scheme across your codebase so that future collaborators can easily locate related logic.
42-75: Consider merging sort and filter states if needed.
You currently maintain separate states for sorting (sortConfig) and filtering (filters). If these configurations become more complex over time, consider bundling them in a single reducer or context for clarity.
130-139: Improve user feedback for sorting indication.
Your arrow icons clearly indicate sorting direction. For better accessibility, consider adding anaria-sortattribute on the clickable headers to help screen readers identify the current sort state.<TableHead className='cursor-pointer' onClick={() => requestSort('name')} + aria-sort={sortConfig?.key === 'name' ? sortConfig.direction : 'none'}>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
clients/intro_course_developer_component/routes/index.tsx(2 hunks)clients/intro_course_developer_component/sidebar/index.tsx(1 hunks)clients/intro_course_developer_component/src/introCourse/pages/DeveloperProfilesLecturer/DeveloperProfilesLecturerPage.tsx(1 hunks)clients/intro_course_developer_component/src/introCourse/pages/DeveloperProfilesLecturer/components/FilterMenu.tsx(1 hunks)servers/intro_course/developerProfile/router.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- clients/intro_course_developer_component/routes/index.tsx
- clients/intro_course_developer_component/src/introCourse/pages/DeveloperProfilesLecturer/components/FilterMenu.tsx
- clients/intro_course_developer_component/sidebar/index.tsx
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build-servers / intro-course / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-server-intro-course
- GitHub Check: build-clients / clients-base / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-clients-base
- GitHub Check: build-clients / clients-base / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-clients-base
- GitHub Check: build-servers / core / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-server-core
- GitHub Check: build-servers / intro-course / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-server-intro-course
- GitHub Check: build-servers / core / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-server-core
🔇 Additional comments (6)
servers/intro_course/developerProfile/router.go (2)
19-19: LGTM: New route added for developer profile updatesThe new PUT route properly leverages the authentication middleware to restrict access to admin and lecturer roles, which aligns with the architectural pattern used throughout the router.
31-31: Comment wording correctedMinor fix to comment wording from "ger" to "get" improves readability.
clients/intro_course_developer_component/src/introCourse/pages/DeveloperProfilesLecturer/DeveloperProfilesLecturerPage.tsx (4)
1-11: Use consistent import style and maintain logical grouping.
All imports look good and relevant, but consider grouping common dependencies together for consistency and readability (e.g. external libraries first, then local components/utilities).
12-26: Validate icon usage and handle missing icons gracefully if needed.
The icons fromlucide-reactare used properly for better visual feedback. Make sure that the icons are always rendered in consistent sizes and handle any potential scenario where the icon might fail to load or become unavailable in the future (unlikely, but worth a note if icons are externally sourced).
99-120: Error boundary usage is appropriate.
Returning anErrorPagecomponent upon failure with a refresh action is good. Ensure that theErrorPageitself gracefully handles network or server failures and that any advanced error reporting is integrated if your app requires logs or telemetry.
193-267: Ensure that row clicks and “Edit” clicks are handled distinctly.
You calle.stopPropagation()in the button’s click handler—this is a standard approach to separate row-level and button-level actions. Just confirm that your QA team is aware of this subtlety to ensure consistent UX tests.
| // Get the developer profile and course phase participations | ||
| const { phaseId } = useParams<{ phaseId: string }>() | ||
| const { | ||
| data: coursePhaseParticipations, | ||
| isPending: isCoursePhaseParticipationsPending, | ||
| isError: isParticipationsError, | ||
| refetch: refetchCoursePhaseParticipations, | ||
| } = useQuery<CoursePhaseParticipationsWithResolution>({ | ||
| queryKey: ['participants', phaseId], | ||
| queryFn: () => getCoursePhaseParticipations(phaseId ?? ''), | ||
| }) | ||
|
|
||
| const { | ||
| data: developerProfiles, | ||
| isPending: isDeveloperProfilesPending, | ||
| isError: isDeveloperProfileError, | ||
| refetch: refetchDeveloperProfiles, | ||
| } = useQuery<DeveloperProfile[]>({ | ||
| queryKey: ['developerProfiles', phaseId], | ||
| queryFn: () => getAllDeveloperProfiles(phaseId ?? ''), | ||
| }) | ||
|
|
||
| const isError = isParticipationsError || isDeveloperProfileError |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Check for invalid phaseId usage.
You are defaulting phaseId to an empty string if it’s undefined. Double-check that downstream functions (getCoursePhaseParticipations, getAllDeveloperProfiles) handle an empty string gracefully. Eliminate potential edge cases if phaseId is required.
Use this script to search callers for possible usage issues:
🏁 Script executed:
#!/bin/bash
# Searching for all calls to getCoursePhaseParticipations and getAllDeveloperProfiles
rg -A 5 "getCoursePhaseParticipations|getAllDeveloperProfiles"Length of output: 10984
PhaseId Validation Issue in API Queries
It appears that using phaseId ?? '' may inadvertently trigger API calls with an empty string when phaseId is missing. In the backend (e.g., in the Go router for getAllDeveloperProfiles), an empty string will cause uuid.Parse to fail, returning a 400 error. This pattern is found in multiple components. To address this issue, please ensure that:
-
The queries are not initiated when
phaseIdis falsy. For example, update your React Query configurations using theenabledoption:useQuery({ queryKey: ['participants', phaseId], queryFn: () => getCoursePhaseParticipations(phaseId!), enabled: !!phaseId, })
-
Downstream functions such as
getCoursePhaseParticipationsandgetAllDeveloperProfilesare either updated to handle an empty string gracefully or are protected by ensuring that they are called only whenphaseIdis valid.
Review these changes thoroughly to eliminate potential edge cases when phaseId is required.
niclasheun
requested a deployment
to
prompt-dev-vm
GitHub Actions
Waiting
This PR introduces a Lecturer View for the Developer Profile.
This does not yet include the possibility to automatically create Gitlab Projects or add students to the apple developer account, but this will follow in the next PR.
Summary by CodeRabbit
New Features
Bug Fixes