Assessment: Mark Submission as Final
#411
Conversation
WalkthroughThis update introduces comprehensive support for tracking and managing assessment completion status for students in a course phase. It adds new backend APIs, database tables, SQL queries, and services for marking, unmarking, and querying assessment completion. Corresponding TypeScript types, hooks, and UI logic are implemented on the client side, including new hooks for fetching and mutating assessment completion, and UI changes to reflect and control completion status. The OpenAPI specification and DTOs are updated to aggregate assessments, remaining assessments, and completion status into unified structures. Related router logic and component props are refactored for consistency. Changes
Sequence Diagram(s)sequenceDiagram
participant UI
participant ClientAPI
participant ServerAPI
participant DB
UI->>ClientAPI: Fetch StudentAssessment (phaseId, participationId)
ClientAPI->>ServerAPI: GET /student-assessment/:coursePhaseID/:courseParticipationID
ServerAPI->>DB: Query Assessments, Remaining, Completion
DB-->>ServerAPI: Data
ServerAPI-->>ClientAPI: StudentAssessment DTO
ClientAPI-->>UI: StudentAssessment
UI->>ClientAPI: Mark Assessment as Completed
ClientAPI->>ServerAPI: POST /student-assessment/completed
ServerAPI->>DB: Insert assessment_completion
DB-->>ServerAPI: Success
ServerAPI-->>ClientAPI: 201 Created
UI->>ClientAPI: Unmark Assessment as Completed
ClientAPI->>ServerAPI: DELETE /student-assessment/completed/course-participation/:id
ServerAPI->>DB: Delete assessment_completion
DB-->>ServerAPI: Success
ServerAPI-->>ClientAPI: 200 OK
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (1)clients/assessment_component/src/assessment/pages/AssessmentPage/components/AssessmentProfile.tsx (2)⏰ Context from checks skipped due to timeout of 90000ms (9)
🔇 Additional comments (5)
✨ 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🔭 Outside diff range comments (2)
servers/assessment/docs/openapi.yaml (2)
542-560: 🛠️ Refactor suggestionMissing required fields in CreateCompetencyRequest.
The
CreateCompetencyRequestschema doesn't have any required fields, which could lead to invalid requests being accepted. At minimum,categoryIDandnameshould be required for creating a competency.CreateCompetencyRequest: type: object + required: [categoryID, name] properties: categoryID: type: string format: uuid
612-636: 🛠️ Refactor suggestionMissing required fields in CreateOrUpdateAssessmentRequest.
Similar to the competency request, the
CreateOrUpdateAssessmentRequestschema doesn't have any required fields. Fields likecourseParticipationID,coursePhaseID, andcompetencyIDshould be required.CreateOrUpdateAssessmentRequest: type: object + required: [courseParticipationID, coursePhaseID, competencyID] properties: courseParticipationID: type: string format: uuid
♻️ Duplicate comments (1)
servers/assessment/assessments/assessmentCompletion/router.go (1)
35-46:coursePhaseIDwill always be empty -> UUID parse errorBecause the route does not contain
:coursePhaseID,c.Param("coursePhaseID")returns an empty string, causinguuid.Parseto error and the handler to exit with 400 every time. Fixing the path as shown above resolves this.
🧹 Nitpick comments (23)
clients/assessment_component/src/assessment/pages/SettingsPage/hooks/useCreateCategory.ts (1)
13-15: Turn the TODO into action by broadening cache invalidation nowThe new comment flags a future enhancement, but delaying this can lead to stale category data in the UI. Consider implementing a predicate-based invalidation today to catch all category-related queries:
- // update this to invalidate all queries that are related to categories - queryClient.invalidateQueries({ queryKey: ['categories'] }) + // Invalidate any query whose key starts with 'categories' + queryClient.invalidateQueries({ + predicate: query => query.queryKey[0] === 'categories' + })Once this is in place, you can safely remove the TODO.
clients/assessment_component/src/assessment/pages/SettingsPage/hooks/useCreateCompetency.ts (1)
14-16: Implement the TODO: Expand competency queries invalidationThe added comment indicates broader invalidation is needed, but leaving it unimplemented risks stale competency lists. You can replace the current call with a predicate-based invalidation:
- // TODO update this to invalidate all queries that are related to competencies - queryClient.invalidateQueries({ queryKey: ['competencies'] }) + // Invalidate any query whose key starts with 'competencies' + queryClient.invalidateQueries({ + predicate: query => query.queryKey[0] === 'competencies' + })After applying, remove the TODO to avoid confusion.
clients/assessment_component/src/assessment/pages/SettingsPage/hooks/useDeleteCategory.ts (1)
12-14: Action the TODO: Broaden category cache invalidationSimilar to creation, deletion should clear all category-related caches. Update the invalidation call now:
- // TODO update this to invalidate all queries that are related to categories - queryClient.invalidateQueries({ queryKey: ['categories'] }) + // Invalidate any query whose key starts with 'categories' + queryClient.invalidateQueries({ + predicate: query => query.queryKey[0] === 'categories' + })This ensures no stale category data remains. Then drop the TODO.
clients/assessment_component/src/assessment/network/queries/getStudentAssessment.ts (1)
4-17: Consider documenting error handling expectationsWhile letting errors propagate to the caller (typically handled by React Query) is a common pattern, it would be helpful to add a brief comment documenting this expectation for future maintainers.
servers/assessment/db/migration/0003_final_assessment.up.sql (1)
1-10: Table structure looks good but lacks foreign key constraintsThe table design correctly uses a composite primary key and appropriate data types with NOT NULL constraints. However, consider adding foreign key constraints to ensure referential integrity:
CREATE TABLE assessment_completion ( course_participation_id UUID NOT NULL, course_phase_id UUID NOT NULL, completed_at TIMESTAMP NOT NULL, author TEXT NOT NULL, PRIMARY KEY (course_participation_id, course_phase_id) + FOREIGN KEY (course_participation_id) REFERENCES course_participation(id), + FOREIGN KEY (course_phase_id) REFERENCES course_phase(id) );This would prevent orphaned records if a participation or phase is deleted.
clients/assessment_component/src/assessment/pages/AssessmentPage/hooks/useGetStudentAssessment.tsx (1)
12-12: Consider handling missing parameters more explicitlyUsing fallback empty strings (
??'') for potentially undefined parameters might mask routing issues. Consider adding validation or throwing a meaningful error if these required parameters are missing.- queryFn: () => getStudentAssessment(phaseId ?? '', courseParticipationID ?? ''), + queryFn: () => { + if (!phaseId || !courseParticipationID) { + throw new Error('Missing required parameters: phaseId and courseParticipationID must be provided'); + } + return getStudentAssessment(phaseId, courseParticipationID); + },servers/assessment/assessments/remainingAssessments/main.go (2)
10-16: Consider an alternative to the singleton patternThe current implementation uses a global singleton which can make testing difficult and creates hidden dependencies. Consider returning the service instance instead:
-func InitRemainingAssessmentsModule(routerGroup *gin.RouterGroup, queries db.Queries, conn *pgxpool.Pool) { +func InitRemainingAssessmentsModule(routerGroup *gin.RouterGroup, queries db.Queries, conn *pgxpool.Pool) *RemainingAssessmentsService { setupRemainingAssessmentsRouter(routerGroup, promptSDK.AuthenticationMiddleware) - RemainingAssessmentsServiceSingleton = &RemainingAssessmentsService{ + service := &RemainingAssessmentsService{ queries: queries, conn: conn, } + return service }This approach would:
- Make the code more testable by allowing service mocking
- Make dependencies explicit rather than hidden
- Give callers more control over the service lifecycle
10-16: Consider adding error handlingThe initialization function doesn't return any errors that might occur during setup. If
setupRemainingAssessmentsRouteror the service initialization could fail, consider adding error handling:-func InitRemainingAssessmentsModule(routerGroup *gin.RouterGroup, queries db.Queries, conn *pgxpool.Pool) { +func InitRemainingAssessmentsModule(routerGroup *gin.RouterGroup, queries db.Queries, conn *pgxpool.Pool) error { - setupRemainingAssessmentsRouter(routerGroup, promptSDK.AuthenticationMiddleware) + if err := setupRemainingAssessmentsRouter(routerGroup, promptSDK.AuthenticationMiddleware); err != nil { + return err + } RemainingAssessmentsServiceSingleton = &RemainingAssessmentsService{ queries: queries, conn: conn, } + return nil }clients/assessment_component/src/assessment/network/mutations/deleteAssessmentCompletion.ts (1)
3-20: Function implementation looks good, but Content-Type header may be unnecessaryThe implementation follows best practices for asynchronous operations and error handling. However, setting the 'Content-Type' header for a DELETE request is typically unnecessary since DELETE requests generally don't have request bodies.
return await assessmentAxiosInstance.delete( `assessment/api/course_phase/${coursePhaseID}/student-assessment/completed/course-participation/${courseParticipationID}`, - { - headers: { - 'Content-Type': 'application/json', - }, - }, + {}, // Empty config object or remove it entirely if no other settings are needed )clients/assessment_component/src/assessment/network/mutations/createAssessmentCompletion.ts (1)
4-22: Function implementation is sound but return value could be more explicitThe implementation follows TypeScript best practices, but the function awaits the axios call without returning its result, implicitly returning void. Consider either explicitly returning void or returning the response data for consistency.
export const createAssessmentCompletion = async ( coursePhaseID: string, assessmentCompletion: AssessmentCompletion, ): Promise<void> => { try { - await assessmentAxiosInstance.post<AssessmentCompletion>( + return await assessmentAxiosInstance.post<AssessmentCompletion>( `assessment/api/course_phase/${coursePhaseID}/student-assessment/completed`, assessmentCompletion, { headers: { 'Content-Type': 'application/json', }, }, ) } catch (err) { console.error(err) throw err } }Alternatively, if you truly want to return void and not the response:
export const createAssessmentCompletion = async ( coursePhaseID: string, assessmentCompletion: AssessmentCompletion, ): Promise<void> => { try { await assessmentAxiosInstance.post<AssessmentCompletion>( `assessment/api/course_phase/${coursePhaseID}/student-assessment/completed`, assessmentCompletion, { headers: { 'Content-Type': 'application/json', }, }, ) + return } catch (err) { console.error(err) throw err } }clients/assessment_component/src/assessment/pages/AssessmentPage/hooks/useCreateAssessmentCompletion.tsx (1)
1-27: Hook implementation looks good, but there's room for type safety improvementsThe React hook is well-structured and handles the creation of assessment completion with proper query invalidation and error management. However, there are a couple of improvements that could be made:
- return useMutation({ - mutationFn: (assessmentCompletion: AssessmentCompletion) => { - return createAssessmentCompletion(phaseId ?? '', assessmentCompletion) + return useMutation({ + mutationFn: (assessmentCompletion: AssessmentCompletion) => { + if (!phaseId) { + throw new Error('Phase ID is required') + } + return createAssessmentCompletion(phaseId, assessmentCompletion)Also consider using a more specific type for error handling:
- onError: (error: any) => { + onError: (error: unknown) => {clients/assessment_component/src/assessment/pages/AssessmentPage/hooks/useDeleteAssessmentCompletion.tsx (1)
5-25: Consider adding validation and improving type safetyThe hook is well-structured but has similar issues to the
useCreateAssessmentCompletionhook:- mutationFn: (courseParticipationID: string) => - deleteAssessmentCompletion(phaseId ?? '', courseParticipationID), + mutationFn: (courseParticipationID: string) => { + if (!phaseId) { + throw new Error('Phase ID is required') + } + return deleteAssessmentCompletion(phaseId, courseParticipationID) + },Also replace
anywith a more specific type:- onError: (error: any) => { + onError: (error: unknown) => {Additionally, there's significant duplication between this hook and
useCreateAssessmentCompletion. Consider extracting the common error handling logic into a shared utility function.servers/assessment/assessments/assessmentDTO/studentAssessment.go (1)
9-14: Add field documentation for better maintainabilityThe
StudentAssessmentstruct properly aggregates assessment information, but would benefit from documentation comments on each field to clarify their purpose and whether they're optional or required.type StudentAssessment struct { + // CourseParticipationID uniquely identifies the student's participation in the course CourseParticipationID uuid.UUID `json:"courseParticipationID"` + // Assessments contains all assessments for this student in the current phase Assessments []Assessment `json:"assessments"` + // RemainingAssessments tracks which assessments still need to be completed RemainingAssessments remainingAssessmentsDTO.RemainingAssessments `json:"remainingAssessments"` + // AssessmentCompletion contains information about the completion status of assessments AssessmentCompletion assessmentCompletionDTO.AssessmentCompletion `json:"assessmentCompletion"` }clients/assessment_component/src/assessment/pages/AssessmentPage/AssessmentPage.tsx (1)
27-34: Consider adding type checking for studentAssessment.While the condition checks for existence, consider adding type guards or more specific null checks to prevent potential runtime errors.
-if (!studentAssessment) { +if (!studentAssessment || typeof studentAssessment !== 'object') {clients/assessment_component/src/assessment/pages/AssessmentPage/components/AssessmentForm.tsx (1)
51-54: Consider combining hook declarations for clarity.The hooks are split into variables but immediately combined. Consider using a single declaration for improved readability.
- const updateAssessment = useUpdateAssessment(setError) - const createAssessment = useCreateAssessment(setError) - const { mutate } = assessment ? updateAssessment : createAssessment + const { mutate } = assessment + ? useUpdateAssessment(setError) + : useCreateAssessment(setError)servers/assessment/db/query/assessment_completion.sql (1)
16-18: **Consider explicitly specifying columns instead of using SELECT ***While
SELECT *works, explicitly listing the columns (as done in theGetAssessmentCompletionquery) is generally considered better practice for maintainability and performance.-SELECT * FROM assessment_completion +SELECT course_participation_id, course_phase_id, completed_at, author FROM assessment_completionservers/assessment/assessments/service.go (2)
107-142: Consider optimizing multiple database calls in GetStudentAssessmentThe function makes 3-4 separate database calls which could potentially be optimized. Consider combining these queries or using a transaction to reduce database round trips for better performance.
While the current implementation is functionally correct, it could be optimized by:
- Using a database transaction
- Potentially combining some of these queries into a join operation
- Using batch fetching if possible
This would be especially beneficial when this endpoint is used frequently or for courses with many students.
198-212: Improve the error message in checkAssessmentCompletionExistsThe generic error message "assessment was already marked as completed" doesn't provide specific context about which assessment (competency, student) was completed.
- return errors.New("assessment was already marked as completed") + return fmt.Errorf("assessment for student %s in course phase %s was already marked as completed", + courseParticipationID, coursePhaseID)servers/assessment/assessments/remainingAssessments/router.go (1)
19-29: Inconsistent parameter extraction order
coursePhaseIDis extracted first followed bycourseParticipationID, which is inconsistent with how other handlers in the router.go file handle these parameters. Consider maintaining consistency with other handlers.- coursePhaseID, err := uuid.Parse(c.Param("coursePhaseID")) - if err != nil { - handleError(c, http.StatusBadRequest, err) - return - } - courseParticipationID, err := uuid.Parse(c.Param("courseParticipationID")) + courseParticipationID, err := uuid.Parse(c.Param("courseParticipationID")) + if err != nil { + handleError(c, http.StatusBadRequest, err) + return + } + coursePhaseID, err := uuid.Parse(c.Param("coursePhaseID"))servers/assessment/assessments/assessmentCompletion/router.go (1)
21-21: Typo in handler name
markAssessmenAsCompleteis missing a “t”. Consider renaming tomarkAssessmentAsCompletefor clarity & consistency.servers/assessment/assessments/assessmentCompletion/service.go (1)
57-66:CompletedAtsource ignores client‑supplied timestamp &completedflagYou overwrite
CompletedAtwithtime.Now()and ignorereq.CompletedAt/req.Completed.
If the API contract allows the client to send these fields (DTO includes both), either:
- Trust the incoming values (after validation), or
- Remove them from the DTO to avoid ambiguity.
At minimum document the server‑authoritative behaviour to prevent misuse.
servers/assessment/docs/openapi.yaml (1)
273-273: Fix typo in endpoint summary.There's a typo in the endpoint summary: "Get the student assessment fo an object" should be "Get the student assessment for a student".
- summary: Get the student assessment fo an object in a course phase + summary: Get the student assessment for a student in a course phaseservers/assessment/db/sqlc/assessment_completion.sql.go (1)
90-116:❓ Verification inconclusive
Add unique constraint verification.
The
MarkAssessmentAsFinishedfunction inserts a record directly without checking if it already exists. Consider adding error handling for unique constraint violations to handle potential concurrent submissions gracefully.
🏁 Script executed:
#!/bin/bash # Check if there's a unique constraint on the assessment_completion table # Look for migration files defining the table fd -e sql -e SQL "assessment_completion" --exec grep -i "CONSTRAINT\|UNIQUE\|PRIMARY KEY" {} \;Length of output: 98
🏁 Script executed:
#!/bin/bash # Search for table creation and constraints/indexes on assessment_completion set -e # Find CREATE TABLE definitions rg -i 'create\s+table\s+assessment_completion' -n # Find UNIQUE INDEX definitions on the table rg -i 'unique\s+index\s+.*assessment_completion' -n # Find unique constraints involving course_participation_id and course_phase_id rg -i 'unique\s*\(.*course_participation_id.*course_phase_id' -n # Find any constraint keywords referencing assessment_completion rg -i 'constraint\b.*assessment_completion' -nLength of output: 60
Verify existence of unique constraint on assessment_completion
I wasn’t able to find any UNIQUE constraint or index on(course_participation_id, course_phase_id)in your SQL migrations. Please confirm that the database enforces uniqueness on these two columns (or add one if missing) to prevent duplicate assessment completions. Once that’s in place, wrap the insert in error handling for PostgreSQL’s unique‐violation error (SQLSTATE 23505) so concurrent attempts are handled gracefully.• Check your migration that creates
assessment_completionfor a line like:ALTER TABLE assessment_completion ADD CONSTRAINT uq_assessment_completion_participation_phase UNIQUE (course_participation_id, course_phase_id);• In
MarkAssessmentAsFinished, catchpgconn.PgErrorwith.Code == "23505"and return a domain‑specific error (e.g.,ErrAlreadyFinished).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
clients/assessment_component/src/assessment/interfaces/assessment.ts(1 hunks)clients/assessment_component/src/assessment/interfaces/studentAssessment.ts(1 hunks)clients/assessment_component/src/assessment/network/mutations/createAssessmentCompletion.ts(1 hunks)clients/assessment_component/src/assessment/network/mutations/deleteAssessmentCompletion.ts(1 hunks)clients/assessment_component/src/assessment/network/queries/getStudentAssessment.ts(1 hunks)clients/assessment_component/src/assessment/pages/AssessmentPage/AssessmentPage.tsx(4 hunks)clients/assessment_component/src/assessment/pages/AssessmentPage/components/AssessmentForm.tsx(7 hunks)clients/assessment_component/src/assessment/pages/AssessmentPage/components/AssessmentProfile.tsx(1 hunks)clients/assessment_component/src/assessment/pages/AssessmentPage/components/CategoryAssessment.tsx(2 hunks)clients/assessment_component/src/assessment/pages/AssessmentPage/hooks/useCreateAssessmentCompletion.tsx(1 hunks)clients/assessment_component/src/assessment/pages/AssessmentPage/hooks/useDeleteAssessmentCompletion.tsx(1 hunks)clients/assessment_component/src/assessment/pages/AssessmentPage/hooks/useGetStudentAssessment.tsx(1 hunks)clients/assessment_component/src/assessment/pages/SettingsPage/hooks/useCreateCategory.ts(1 hunks)clients/assessment_component/src/assessment/pages/SettingsPage/hooks/useCreateCompetency.ts(1 hunks)clients/assessment_component/src/assessment/pages/SettingsPage/hooks/useDeleteCategory.ts(1 hunks)servers/assessment/assessments/assessmentCompletion/assessmentCompletionDTO/assessmentCompletion.go(1 hunks)servers/assessment/assessments/assessmentCompletion/main.go(1 hunks)servers/assessment/assessments/assessmentCompletion/router.go(1 hunks)servers/assessment/assessments/assessmentCompletion/service.go(1 hunks)servers/assessment/assessments/assessmentDTO/studentAssessment.go(1 hunks)servers/assessment/assessments/remainingAssessments/main.go(1 hunks)servers/assessment/assessments/remainingAssessments/remainingAssessmentsDTO/remainingAssessments.go(1 hunks)servers/assessment/assessments/remainingAssessments/router.go(1 hunks)servers/assessment/assessments/remainingAssessments/service.go(1 hunks)servers/assessment/assessments/router.go(2 hunks)servers/assessment/assessments/service.go(5 hunks)servers/assessment/db/migration/0003_final_assessment.up.sql(1 hunks)servers/assessment/db/query/assessment_completion.sql(1 hunks)servers/assessment/db/sqlc/assessment_completion.sql.go(1 hunks)servers/assessment/db/sqlc/models.go(1 hunks)servers/assessment/docs/openapi.yaml(5 hunks)servers/assessment/main.go(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
servers/assessment/assessments/router.go (2)
Learnt from: rappm
PR: ls1intum/prompt2#344
File: servers/assessment/assessments/router.go:20-20
Timestamp: 2025-04-01T12:17:59.359Z
Learning: In the assessment server, the coursePhaseID parameter is already included in the base router path from the main.go file (api := router.Group("assessment/api/course_phase/:coursePhaseID")), so routes defined in module routers don't need to explicitly include it in their paths.
Learnt from: rappm
PR: ls1intum/prompt2#392
File: servers/assessment/assessments/router.go:176-195
Timestamp: 2025-04-10T14:41:59.900Z
Learning: In the assessment server implementation using Gin, the coursePhaseID parameter is declared at the router group level, making it accessible to all child routes without needing to redeclare it in each subendpoint handler.
🧬 Code Graph Analysis (16)
clients/assessment_component/src/assessment/interfaces/assessment.ts (2)
servers/assessment/assessments/assessmentCompletion/assessmentCompletionDTO/assessmentCompletion.go (1)
AssessmentCompletion(9-15)servers/assessment/db/sqlc/models.go (1)
AssessmentCompletion(70-75)
servers/assessment/main.go (2)
servers/assessment/assessments/assessmentCompletion/main.go (1)
InitAssessmentCompletionModule(10-16)servers/assessment/assessments/remainingAssessments/main.go (1)
InitRemainingAssessmentsModule(10-16)
servers/assessment/assessments/remainingAssessments/main.go (1)
servers/assessment/assessments/remainingAssessments/service.go (2)
RemainingAssessmentsServiceSingleton(18-18)RemainingAssessmentsService(13-16)
servers/assessment/assessments/assessmentCompletion/main.go (1)
servers/assessment/assessments/assessmentCompletion/service.go (2)
AssessmentCompletionServiceSingleton(24-24)AssessmentCompletionService(19-22)
clients/assessment_component/src/assessment/interfaces/studentAssessment.ts (3)
servers/assessment/assessments/assessmentDTO/studentAssessment.go (1)
StudentAssessment(9-14)clients/assessment_component/src/assessment/interfaces/assessment.ts (3)
Assessment(3-12)RemainingAssessmentsForStudent(29-32)AssessmentCompletion(34-40)servers/assessment/assessments/assessmentCompletion/assessmentCompletionDTO/assessmentCompletion.go (1)
AssessmentCompletion(9-15)
clients/assessment_component/src/assessment/network/mutations/createAssessmentCompletion.ts (2)
clients/assessment_component/src/assessment/interfaces/assessment.ts (1)
AssessmentCompletion(34-40)servers/assessment/assessments/assessmentCompletion/assessmentCompletionDTO/assessmentCompletion.go (1)
AssessmentCompletion(9-15)
servers/assessment/assessments/assessmentCompletion/assessmentCompletionDTO/assessmentCompletion.go (2)
clients/assessment_component/src/assessment/interfaces/assessment.ts (1)
AssessmentCompletion(34-40)servers/assessment/db/sqlc/models.go (1)
AssessmentCompletion(70-75)
servers/assessment/assessments/assessmentDTO/studentAssessment.go (3)
clients/assessment_component/src/assessment/interfaces/assessment.ts (2)
Assessment(3-12)AssessmentCompletion(34-40)servers/assessment/assessments/remainingAssessments/remainingAssessmentsDTO/remainingAssessments.go (1)
RemainingAssessments(16-19)servers/assessment/assessments/assessmentCompletion/assessmentCompletionDTO/assessmentCompletion.go (1)
AssessmentCompletion(9-15)
clients/assessment_component/src/assessment/pages/AssessmentPage/hooks/useCreateAssessmentCompletion.tsx (3)
clients/assessment_component/src/assessment/interfaces/assessment.ts (1)
AssessmentCompletion(34-40)servers/assessment/assessments/assessmentCompletion/assessmentCompletionDTO/assessmentCompletion.go (1)
AssessmentCompletion(9-15)clients/assessment_component/src/assessment/network/mutations/createAssessmentCompletion.ts (1)
createAssessmentCompletion(4-22)
servers/assessment/assessments/remainingAssessments/service.go (1)
servers/assessment/db/sqlc/assessment.sql.go (2)
CountRemainingAssessmentsForStudentRow(50-53)CountRemainingAssessmentsForStudentParams(45-48)
clients/assessment_component/src/assessment/network/queries/getStudentAssessment.ts (2)
servers/assessment/assessments/assessmentDTO/studentAssessment.go (1)
StudentAssessment(9-14)clients/assessment_component/src/assessment/interfaces/studentAssessment.ts (1)
StudentAssessment(3-8)
clients/assessment_component/src/assessment/pages/AssessmentPage/hooks/useDeleteAssessmentCompletion.tsx (1)
clients/assessment_component/src/assessment/network/mutations/deleteAssessmentCompletion.ts (1)
deleteAssessmentCompletion(3-20)
servers/assessment/assessments/service.go (8)
servers/assessment/assessments/assessmentDTO/studentAssessment.go (1)
StudentAssessment(9-14)servers/assessment/assessments/remainingAssessments/service.go (1)
CountRemainingAssessmentsForStudent(20-30)servers/assessment/assessments/assessmentCompletion/service.go (2)
CheckAssessmentCompletionExists(26-36)GetAssessmentCompletion(97-107)servers/assessment/assessments/assessmentCompletion/assessmentCompletionDTO/assessmentCompletion.go (2)
AssessmentCompletion(9-15)MapDBAssessmentCompletionToAssessmentCompletionDTO(26-34)servers/assessment/db/sqlc/models.go (1)
AssessmentCompletion(70-75)servers/assessment/assessments/assessmentDTO/assessment.go (1)
GetAssessmentDTOsFromDBModels(23-38)servers/assessment/assessments/remainingAssessments/remainingAssessmentsDTO/remainingAssessments.go (2)
RemainingAssessments(16-19)MapToRemainingAssessmentsDTO(21-34)servers/assessment/db/sqlc/assessment_completion.sql.go (1)
CheckAssessmentCompletionExistsParams(24-27)
servers/assessment/db/sqlc/assessment_completion.sql.go (4)
servers/assessment/assessments/assessmentCompletion/service.go (2)
CheckAssessmentCompletionExists(26-36)GetAssessmentCompletion(97-107)clients/assessment_component/src/assessment/interfaces/assessment.ts (1)
AssessmentCompletion(34-40)servers/assessment/assessments/assessmentCompletion/assessmentCompletionDTO/assessmentCompletion.go (1)
AssessmentCompletion(9-15)servers/assessment/db/sqlc/models.go (1)
AssessmentCompletion(70-75)
servers/assessment/assessments/assessmentCompletion/router.go (4)
clients/assessment_component/src/assessment/interfaces/assessment.ts (1)
AssessmentCompletion(34-40)servers/assessment/assessments/assessmentCompletion/assessmentCompletionDTO/assessmentCompletion.go (1)
AssessmentCompletion(9-15)servers/assessment/db/sqlc/models.go (1)
AssessmentCompletion(70-75)servers/assessment/assessments/assessmentCompletion/service.go (3)
MarkAssessmentAsCompleted(38-74)UnmarkAssessmentAsCompleted(76-86)GetAssessmentCompletion(97-107)
servers/assessment/assessments/remainingAssessments/router.go (2)
servers/assessment/assessments/remainingAssessments/service.go (1)
CountRemainingAssessmentsForStudent(20-30)servers/assessment/assessments/remainingAssessments/remainingAssessmentsDTO/remainingAssessments.go (1)
MapToRemainingAssessmentsDTO(21-34)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- 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 / assessment / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-server-assessment
- GitHub Check: build-servers / assessment / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-server-assessment
- 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
- GitHub Check: build-servers / team-allocation / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-server-team-allocation
- GitHub Check: build-servers / core / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-server-core
- GitHub Check: build-servers / team-allocation / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-server-team-allocation
- GitHub Check: build-servers / intro-course / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-server-intro-course
🔇 Additional comments (44)
servers/assessment/assessments/remainingAssessments/remainingAssessmentsDTO/remainingAssessments.go (1)
1-1: Package name change accurately reflects domain contextThe package name change from
assessmentDTOtoremainingAssessmentsDTOaligns well with the module's specific responsibility of handling remaining assessments. This is a good example of well-defined boundaries and clear domain separation.servers/assessment/main.go (2)
14-15: LGTM: New module importsThe imports for the new assessment completion and remaining assessments modules are properly added.
94-95: LGTM: Proper module initializationThe new modules are correctly initialized following the established pattern in the codebase. Each module receives the same dependencies as other modules: API router group, database queries, and connection pool.
servers/assessment/db/sqlc/models.go (1)
70-75: LGTM: Database model for assessment completionThe
AssessmentCompletionstruct is well-designed with appropriate fields to track when and by whom an assessment was marked as completed for a specific course participant and phase.clients/assessment_component/src/assessment/interfaces/assessment.ts (1)
34-40: LGTM: Client-side interface for assessment completionThe TypeScript interface aligns well with the backend DTO structure while adding a
completedboolean field for easier state management in the UI. The interface maintains consistent naming conventions and type annotations with the rest of the codebase.clients/assessment_component/src/assessment/network/queries/getStudentAssessment.ts (1)
4-17: Well-structured API query functionThe implementation follows best practices for TypeScript API calls with proper typing. The function correctly uses the API endpoint structure and returns the StudentAssessment type as expected.
clients/assessment_component/src/assessment/interfaces/studentAssessment.ts (1)
3-8: Well-structured type definition with good alignment to backendThe
StudentAssessmenttype is clearly defined and properly consolidates the different aspects of a student's assessment status (assessments, remaining assessments, and completion status). The structure exactly mirrors the backend DTO, which will ensure consistent data handling across the stack.servers/assessment/assessments/assessmentCompletion/main.go (1)
10-16: Clean module initialization with proper dependency injectionThe module initialization follows good Go practices including dependency injection and single responsibility. The singleton pattern for the service is appropriate here and the authentication middleware setup ensures proper security.
clients/assessment_component/src/assessment/pages/AssessmentPage/components/CategoryAssessment.tsx (1)
14-14: LGTM! Properly adding completion supportThe
completedprop has been correctly added to the interface and is properly passed down to theAssessmentFormcomponent. This change aligns well with the assessment completion feature being implemented.Also applies to: 21-21, 65-65
servers/assessment/assessments/remainingAssessments/service.go (3)
13-16: Service structure looks good.The
RemainingAssessmentsServiceis well-defined with clear dependencies: database queries and a connection pool. This separation allows for clean testing and dependency injection.
20-30: Error handling approach is appropriate.The function correctly logs the detailed error but returns a generic error message to the caller. This is a good practice for security and usability.
18-18:✅ Verification successful
Verify singleton initialization.
The singleton pattern is used here, but initialization logic isn't shown in this file. Ensure the singleton is properly initialized during application startup.
🏁 Script executed:
#!/bin/bash # Check for singleton initialization in the application rg -A 3 "RemainingAssessmentsServiceSingleton.*=" --glob "*.go" -g "!*_test.go"Length of output: 431
Singleton Initialization Verified
The
RemainingAssessmentsServiceSingletonis properly initialized inservers/assessment/assessments/remainingAssessments/main.goduring application startup:• Line in main.go:
RemainingAssessmentsServiceSingleton = &RemainingAssessmentsService{ queries: queries, conn: conn, }No further action needed.
servers/assessment/assessments/assessmentCompletion/assessmentCompletionDTO/assessmentCompletion.go (3)
9-15: DTO structure well-defined with proper JSON field names.The camelCase JSON field names are appropriate for frontend compatibility, matching the TypeScript interface naming convention in the client code.
17-24: Clean implementation of the conversion function.The function efficiently converts database models to DTOs using a capacity-initialized slice for optimal memory usage.
26-34:✅ Verification successful
Verify the hardcoded 'Completed' value.
The function always sets
Completed: trueregardless of input. This appears intentional since the DB model doesn't have aCompletedfield, suggesting that existence in the database implies completion.
🏁 Script executed:
#!/bin/bash # Check for any usage or conditions of the Completed field rg -B 2 -A 2 "Completed.*:" --type go --glob "assessmentCompletion/*.go" # Check the DB model definition for further context rg -A 10 "AssessmentCompletion struct" --type goLength of output: 2856
No change needed for
Completed– it’s intentionally always true
The DTO maps only completed assessments (the DB model omits aCompletedflag, and existence in the table implies completion). If you later need to represent incomplete records, you can deriveCompletedfromdbAssessment.CompletedAt.Status(orValid), but for now this hard‐coding is correct.clients/assessment_component/src/assessment/pages/AssessmentPage/AssessmentPage.tsx (4)
4-5: Good refactor to consolidated data fetching.Replacing multiple specific hooks with a single
useGetStudentAssessmenthook simplifies the component and improves state management.
20-25: Clean state variable renaming for consistency.The renamed state variables clearly indicate their association with the unified
studentAssessmentdata structure.
36-42: Error and loading states properly handled.The simplified error and loading states are well implemented with appropriate UI feedback.
48-50:✅ Verification successful
Ensure participant prop is still necessary.
The component still requires
participantfor theAssessmentProfile, but it's now paired withstudentAssessment. Verify that both are required or consider refactoring further.
🏁 Script executed:
#!/bin/bash # Check how AssessmentProfile uses both props rg -A 15 "AssessmentProfileProps|interface.*AssessmentProfile" --glob "*.tsx"Length of output: 3368
🏁 Script executed:
rg -n "participant\." clients/assessment_component/src/assessment/pages/AssessmentPage/components/AssessmentProfile.tsxLength of output: 770
Participant Prop Is Required
We’ve confirmed that
participantis used extensively inAssessmentProfile.tsx—for mutation IDs, restricted/student‑readable data, and rendering student details—so it must remain alongsidestudentAssessment.• Key usages in
• Lines 72–76: IDs & data for mutations
• Lines 96, 113, 120–122, 130: Displaying student informationclients/assessment_component/src/assessment/pages/AssessmentPage/components/AssessmentForm.tsx (9)
5-5: Good icon selection for improved UX.Adding the
LockIconprovides clear visual feedback for the completed state.
20-25: Props interface properly extended.The optional
completedprop with appropriate type is a clean addition to the interface.
27-32: Default value for completed prop is sensible.Setting
completed = falseas the default ensures backward compatibility with existing usage.
56-69: Early return in effect is well implemented.The early return when
completedis true prevents unnecessary form submissions, and the dependency array correctly includes thecompletedstate.
71-74: Consistent handling of completed state.The handler correctly prevents score changes when in completed state.
78-83: Clear visual indication of completed state.The styling changes and additional UI elements effectively communicate the completed state to users.
Also applies to: 90-95
108-117: Comprehensive button state handling.The button's disabled state, styling, and cursor changes provide consistent feedback for the completed state.
138-149: Textarea properly disabled when completed.The textarea is correctly set as both
disabledandreadOnlywith appropriate styling changes.
151-152: Form validation messages properly hidden when completed.Error and validation messages are appropriately hidden in the completed state, keeping the UI clean.
Also applies to: 165-170
servers/assessment/db/query/assessment_completion.sql (2)
1-9: Well-structured SQL query for marking an assessment as completedThis query correctly inserts a record with all necessary fields (course_participation_id, course_phase_id, completed_at, author) to track assessment completion status.
26-32: Efficient exists check using EXISTS clauseGood use of the EXISTS clause for efficiently checking if a record exists without retrieving the full record data.
servers/assessment/assessments/service.go (1)
30-33: Good validation to prevent changes to completed assessmentsAdding a check to prevent modification of assessments that have been marked as completed is a solid implementation that maintains data integrity.
servers/assessment/assessments/remainingAssessments/router.go (1)
40-43: Good reusable error handling functionCreating a dedicated error handling function that logs errors and returns formatted JSON responses is a good practice for consistency and code reuse.
servers/assessment/assessments/router.go (2)
69-86: Successfully implemented comprehensive student assessment data retrievalThe
getStudentAssessmenthandler properly retrieves and combines assessment data, remaining assessments count, and completion status into a unified response, which simplifies client-side data handling.
19-19:❌ Incorrect review comment
Potential route conflict with overlapping patterns
The route
/:courseParticipationID(line 19) may conflict with/course-participation/:courseParticipationID(line 20) as they match similar URL patterns. Consider renaming one of these routes to ensure distinct matching patterns.
🏁 Script executed:
#!/bin/bash # Check if there are other route definitions in the main setup file cd servers/assessment # Look for router setup patterns rg "routerGroup.*Group" --type go -A 5 -B 5 # Look for the path patterns in use throughout the codebase rg "courseParticipationID.*Param" --type go -A 2 -B 2Length of output: 11427
No route conflict detected
The two routes live under different router groups—
assessmentRouter.GET("/:courseParticipationID")is mounted on/student-assessment/:courseParticipationIDassessmentCompletionRouter.GET("/course-participation/:courseParticipationID")is mounted on/student-assessment/completed/course-participation/:courseParticipationIDGin always matches static segments (e.g.
completed) before named parameters, so requests to/student-assessment/completed/...will never be captured by the/student-assessment/:courseParticipationIDroute. No renaming is necessary.Likely an incorrect or invalid review comment.
clients/assessment_component/src/assessment/pages/AssessmentPage/components/AssessmentProfile.tsx (1)
64-64: Potential emptycoursePhaseIDsent to the API
phaseId ?? ''falls back to an empty string which will be rejected by the backend (uuid.Parse will fail).
Guard the component earlier (e.g., do not render the “Mark final” button without a validphaseId) or throw a descriptive error.- coursePhaseID: phaseId ?? '', + coursePhaseID: phaseId!,servers/assessment/assessments/assessmentCompletion/router.go (1)
16-19:⚠️ Potential issueRoute paths are missing
coursePhaseID– all requests will 400
unmarkAssessmentAsCompletedandgetAssessmentCompletionexpect bothcourseParticipationIDandcoursePhaseID, yet the registered paths only expose:courseParticipationID.- assessmentRouter.GET("/course-participation/:courseParticipationID", …) - assessmentRouter.DELETE("/course-participation/:courseParticipationID", …) + assessmentRouter.GET("/course-phase/:coursePhaseID/course-participation/:courseParticipationID", …) + assessmentRouter.DELETE("/course-phase/:coursePhaseID/course-participation/:courseParticipationID", …)Aligning path & param parsing is required for the handlers to work.
⛔ Skipped due to learnings
Learnt from: rappm PR: ls1intum/prompt2#344 File: servers/assessment/assessments/router.go:20-20 Timestamp: 2025-04-01T12:17:59.359Z Learning: In the assessment server, the coursePhaseID parameter is already included in the base router path from the main.go file (api := router.Group("assessment/api/course_phase/:coursePhaseID")), so routes defined in module routers don't need to explicitly include it in their paths.Learnt from: mathildeshagl PR: ls1intum/prompt2#406 File: servers/team_allocation/allocations/router.go:69-89 Timestamp: 2025-04-15T16:42:55.313Z Learning: In the team_allocation component (like intro_course and assessment), the coursePhaseID parameter is already included in the base router path from the main.go file, so routes defined in module routers don't need to explicitly include it in their paths.Learnt from: mathildeshagl PR: ls1intum/prompt2#406 File: servers/team_allocation/allocations/router.go:53-67 Timestamp: 2025-04-15T16:44:04.015Z Learning: In the team_allocation component, similar to other components like intro_course and assessment, the coursePhaseID parameter is already included in the base router path defined in main.go (api := router.Group("team-allocation/api/course_phase/:coursePhaseID")), so routes defined in module routers don't need to explicitly include it in their paths.servers/assessment/docs/openapi.yaml (3)
638-647: LGTM!The
RemainingAssessmentsschema is well-structured and provides a clear representation of remaining assessments with their associated categories.
648-664: LGTM!The
AssessmentCompletionschema correctly defines all necessary fields for tracking assessment completion status, including timestamps and authorship information.
665-686: LGTM!The
StudentAssessmentschema provides a comprehensive structure that aggregates all relevant data for a student's assessment status (apart from the schema structure issues mentioned earlier).servers/assessment/db/sqlc/assessment_completion.sql.go (5)
15-34: LGTM!The
CheckAssessmentCompletionExistsquery and implementation are well-structured. The SQL query correctly checks for existence using a composite key of course participation ID and course phase ID.
36-58: LGTM!The
GetAssessmentCompletionfunction is implemented correctly. It efficiently retrieves a single record based on the composite key and maps all fields to the appropriate struct properties.
60-88: LGTM!The
GetAssessmentCompletionsByCoursePhasefunction is well-implemented with proper error handling and resource cleanup (deferring rows.Close()). It correctly scans multiple rows into a slice of AssessmentCompletion structs.
118-132: LGTM!The
UnmarkAssessmentAsFinishedfunction is implemented correctly. It efficiently deletes a record based on the composite key of course participation ID and course phase ID.
1-6: LGTM!The file is properly generated by sqlc with appropriate header comments indicating that manual edits should not be made.
...s/assessment_component/src/assessment/pages/AssessmentPage/hooks/useGetStudentAssessment.tsx
Outdated
Show resolved
Hide resolved
clients/assessment_component/src/assessment/pages/AssessmentPage/AssessmentPage.tsx
Outdated
Show resolved
Hide resolved
...ts/assessment_component/src/assessment/pages/AssessmentPage/components/AssessmentProfile.tsx
Outdated
Show resolved
Hide resolved
...ts/assessment_component/src/assessment/pages/AssessmentPage/components/AssessmentProfile.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
clients/assessment_component/src/assessment/pages/AssessmentPage/components/AssessmentProfile.tsx (2)
72-72: Consider more granular pass status determination.The code currently sets
PassStatus.PASSEDwhen marking an assessment as final, without considering the actual assessment scores. This might not accurately reflect all student performance scenarios.Consider deriving the pass status based on the actual assessment scores or implementing a more granular decision model:
- passStatus = PassStatus.PASSED + // Determine pass status based on assessment scores + passStatus = determinePassStatus(studentAssessment.assessments)You would need to implement a
determinePassStatusfunction that analyzes the assessment scores to determine an appropriate pass status.
93-113: Card header layout might break on very small screens.The current layout combines multiple responsive elements and might still have alignment issues on very small screens, especially with long student names.
Consider adding more specific breakpoints or fluid spacing to ensure the button and status badges don't overlap, particularly when the student name is long.
servers/assessment/assessments/assessmentCompletion/router.go (2)
21-21: Typo in function name.There's a typo in the function name -
markAssessmenAsCompleteis missing a 't'.- func markAssessmenAsComplete(c *gin.Context) { + func markAssessmentAsComplete(c *gin.Context) {Remember to update the reference to this function in the router setup if you make this change.
72-75: Consider adding structured error response.The error handling function is simple and functional, but could be improved to provide more structured error responses with error codes.
Consider enhancing the error response structure:
- func handleError(c *gin.Context, statusCode int, err error) { - log.Error(err) - c.JSON(statusCode, gin.H{"error": err.Error()}) + func handleError(c *gin.Context, statusCode int, err error) { + log.Error(err) + c.JSON(statusCode, gin.H{ + "error": err.Error(), + "code": statusCode, + "timestamp": time.Now().UTC().Format(time.RFC3339), + }) }servers/assessment/docs/openapi.yaml (3)
273-273: Typo in endpoint summary.The summary has a typo: "fo an object" should be "for an object".
- summary: Get the student assessment fo an object in a course phase + summary: Get the student assessment for an object in a course phase
674-677: Consider describing assessment completion semantics in schema description.The new schema properties for assessment completion lack descriptions explaining their purpose and behavior. Adding descriptions would improve API documentation for consumers.
Consider adding descriptions to these properties, for example:
remainingAssessments: $ref: "#/components/schemas/RemainingAssessments" + description: "Information about pending assessments for this student" assessmentCompletion: $ref: "#/components/schemas/AssessmentCompletion" + description: "Status of assessment completion, when marked as final"
405-405: AssessmentCompletion request body could benefit from example value.For better developer experience, consider adding an example value for the AssessmentCompletion request body.
Add an example:
$ref: "#/components/schemas/AssessmentCompletion" + example: + coursePhaseID: "3fa85f64-5717-4562-b3fc-2c963f66afa6" + courseParticipationID: "3fa85f64-5717-4562-b3fc-2c963f66afa6" + author: "John Doe" + completed: true
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
clients/assessment_component/src/assessment/pages/AssessmentPage/AssessmentPage.tsx(4 hunks)clients/assessment_component/src/assessment/pages/AssessmentPage/components/AssessmentProfile.tsx(1 hunks)clients/assessment_component/src/assessment/pages/AssessmentPage/hooks/useGetStudentAssessment.tsx(1 hunks)servers/assessment/assessments/assessmentCompletion/router.go(1 hunks)servers/assessment/assessments/remainingAssessments/router.go(1 hunks)servers/assessment/docs/openapi.yaml(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- clients/assessment_component/src/assessment/pages/AssessmentPage/AssessmentPage.tsx
- clients/assessment_component/src/assessment/pages/AssessmentPage/hooks/useGetStudentAssessment.tsx
- servers/assessment/assessments/remainingAssessments/router.go
🧰 Additional context used
🧠 Learnings (1)
servers/assessment/assessments/assessmentCompletion/router.go (1)
Learnt from: rappm
PR: ls1intum/prompt2#344
File: servers/assessment/assessments/router.go:20-20
Timestamp: 2025-04-01T12:17:59.359Z
Learning: In the assessment server, the coursePhaseID parameter is already included in the base router path from the main.go file (api := router.Group("assessment/api/course_phase/:coursePhaseID")), so routes defined in module routers don't need to explicitly include it in their paths.
🧬 Code Graph Analysis (2)
servers/assessment/assessments/assessmentCompletion/router.go (3)
clients/assessment_component/src/assessment/interfaces/assessment.ts (1)
AssessmentCompletion(34-40)servers/assessment/assessments/assessmentCompletion/assessmentCompletionDTO/assessmentCompletion.go (1)
AssessmentCompletion(9-15)servers/assessment/assessments/assessmentCompletion/service.go (3)
MarkAssessmentAsCompleted(38-74)UnmarkAssessmentAsCompleted(76-86)GetAssessmentCompletion(97-107)
clients/assessment_component/src/assessment/pages/AssessmentPage/components/AssessmentProfile.tsx (6)
clients/assessment_component/src/assessment/interfaces/studentAssessment.ts (1)
StudentAssessment(3-8)clients/assessment_component/src/assessment/pages/AssessmentPage/hooks/useCreateAssessmentCompletion.tsx (1)
useCreateAssessmentCompletion(6-27)clients/assessment_component/src/assessment/pages/AssessmentPage/hooks/useDeleteAssessmentCompletion.tsx (1)
useDeleteAssessmentCompletion(5-25)clients/shared_library/hooks/useUpdateCoursePhaseParticipation.tsx (1)
useUpdateCoursePhaseParticipation(7-39)clients/assessment_component/src/assessment/interfaces/scoreLevel.ts (1)
mapScoreLevelToNumber(8-21)clients/assessment_component/src/assessment/pages/AssessmentPage/components/AssessmentStatusBadge.tsx (1)
AssessmentStatusBadge(9-47)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- 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 / assessment / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-server-assessment
- GitHub Check: build-servers / assessment / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-server-assessment
- 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 / team-allocation / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-server-team-allocation
- GitHub Check: build-servers / team-allocation / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-server-team-allocation
- GitHub Check: build-servers / intro-course / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-server-intro-course
🔇 Additional comments (8)
clients/assessment_component/src/assessment/pages/AssessmentPage/components/AssessmentProfile.tsx (3)
57-87: Improved implementation of assessment completion with proper sequencing.The code now correctly handles the sequential execution of API calls using async/await, properly addressing the race condition from previous review. Additionally, the
completedflag is now correctly set totruewhen marking an assessment as final.
127-132: Good improvement with proper capitalization of degree.The capitalization of the first letter of the study degree makes the profile display more professional and consistent.
150-192: Well-implemented confirmation dialog with clear user guidance.The dialog implementation provides clear user feedback and appropriate visual cues (destructive variant for reopening). The confirmation steps help prevent accidental status changes.
servers/assessment/assessments/assessmentCompletion/router.go (4)
13-19: Route paths follow API design best practices.The router setup follows appropriate patterns with logical grouping and RESTful naming. Based on your learning notes, I see that you're not including coursePhaseID in path since it's already in the base router path.
64-64: Parameter order is now correct.The parameter order has been corrected from the previous review. The service layer function
GetAssessmentCompletionnow receives parameters in the correct order:(c, courseParticipationID, coursePhaseID).
35-51: Consistent parameter acquisition pattern across handlers.Good consistency in how you retrieve and validate UUID parameters across different handlers. This makes the code predictable and easier to maintain.
21-33:✅ Verification successful
Verify business logic for marking assessment as complete.
When marking an assessment as complete, there's no validation in the route handler to ensure all assessments are completed first. This validation appears to be in the service layer (from the code snippets), but consider if it's robust enough.
🏁 Script executed:
#!/bin/bash # Description: Check if assessment completion service validates remaining assessments # Look for validation logic in service implementation rg -A 10 "MarkAssessmentAsCompleted" --type goLength of output: 2352
🏁 Script executed:
#!/bin/bash # Display the MarkAssessmentAsCompleted service function to verify validation and update logic rg -C 20 "func MarkAssessmentAsCompleted" --type go servers/assessment/assessments/assessmentCompletion/service.goLength of output: 1746
Validation Is Handled in the Service Layer
The
MarkAssessmentAsCompletedservice method first callsCountRemainingAssessmentsForStudentand returns an error if any assessments remain, so the route handler does not need additional checks.• No changes are required in servers/assessment/assessments/assessmentCompletion/router.go
• Business logic is already enforced in servers/assessment/assessments/assessmentCompletion/service.goservers/assessment/docs/openapi.yaml (1)
634-677: Schema definitions are now correctly structured.The schema definitions for RemainingAssessments, AssessmentCompletion, and StudentAssessment are now properly structured with correct references using $ref without inappropriate items properties, addressing previous review comments.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
servers/assessment/docs/openapi.yaml (1)
271-304:⚠️ Potential issueFix summary typo and resolve duplicate path definitions for
/student-assessment/{…}
The GET operation on line 273 has a typo ("fo" → "for") and the two separate path definitions/student-assessment/{courseParticipationID} (GET) /student-assessment/{assessmentID} (DELETE)both resolve to
/student-assessment/{…}and will collide in the OpenAPI spec. You can either:
- Merge GET and DELETE under a single path (e.g.
/student-assessment/{studentAssessmentID}) with distinct operations.- Rename one of the paths (e.g.
/student-assessment/assessment/{assessmentID}) to avoid ambiguity.Also update the summary on line 273 to:
summary: Get the student assessment for a course participation
🧹 Nitpick comments (5)
servers/assessment/docs/openapi.yaml (5)
393-395: Simplify response schema for remaining assessments
Rather than wrapping the detailedRemainingAssessmentsobject under a nested property, return it directly:- schema: - type: object - properties: - remainingAssessments: - $ref: "#/components/schemas/RemainingAssessments" + schema: + $ref: "#/components/schemas/RemainingAssessments"This aligns the endpoint response with the
RemainingAssessmentsschema and removes unnecessary nesting.
396-409: Add response schema to POST/student-assessment/completed
The new POST endpoint does not specify a response body. If the API returns the createdAssessmentCompletionobject, document it:responses: "201": description: Assessment marked as completed content: application/json: schema: $ref: "#/components/schemas/AssessmentCompletion"Or, if there’s intentionally no body, switch to a
204 No Contentand note that in the spec.
632-641: AddrequiredtoRemainingAssessmentsschema
All fields inRemainingAssessmentsshould be present. Strengthen the contract:RemainingAssessments: type: object + required: + - remainingAssessments + - categories properties: remainingAssessments: type: integer categories: type: array items: $ref: "#/components/schemas/CategoryWithRemainingAssessments"
642-658: AddrequiredtoAssessmentCompletionschema
Ensure clients supply all necessary fields:AssessmentCompletion: type: object + required: + - coursePhaseID + - courseParticipationID + - completedAt + - author + - completed properties: coursePhaseID: type: string format: uuid courseParticipationID: type: string format: uuid completedAt: type: string format: date-time author: type: string completed: type: boolean
659-676: AddrequiredtoStudentAssessmentschema
Clarify which properties are always returned:StudentAssessment: type: object + required: + - coursePhaseID + - courseParticipationID + - assessments + - remainingAssessments + - assessmentCompletion properties: coursePhaseID: type: string format: uuid courseParticipationID: type: string format: uuid assessments: type: array items: $ref: "#/components/schemas/Assessment" remainingAssessments: $ref: "#/components/schemas/RemainingAssessments" assessmentCompletion: $ref: "#/components/schemas/AssessmentCompletion"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
servers/assessment/docs/openapi.yaml(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: build-servers / team-allocation / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-server-team-allocation
- GitHub Check: build-servers / team-allocation / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-server-team-allocation
- GitHub Check: build-servers / assessment / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-server-assessment
- GitHub Check: build-servers / intro-course / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-server-intro-course
- GitHub Check: build-servers / intro-course / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-server-intro-course
- GitHub Check: build-servers / assessment / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-server-assessment
- GitHub Check: build-servers / core / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-server-core
- GitHub Check: build-servers / core / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-server-core
- GitHub Check: lint-clients / lint-client (devops_challenge_component)
- GitHub Check: lint-clients / lint-client (interview_component)
- GitHub Check: lint-clients / lint-client (team_allocation_component)
- GitHub Check: lint-clients / lint-client (assessment_component)
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
servers/assessment/docs/openapi.yaml (2)
373-381:⚠️ Potential issueRemove or align the unused
coursePhaseIDparameter
The GET/student-assessment/remaining/{courseParticipationID}path only includes{courseParticipationID}in its URL, yet you declare bothcoursePhaseIDandcourseParticipationIDunderparameters. Either:
- Drop the
coursePhaseIDparameter from the spec, or- Include
{coursePhaseID}in the path (e.g./student-assessment/remaining/{coursePhaseID}/{courseParticipationID}) to match your parameters.
390-395:⚠️ Potential issueFlatten the response schema to return
RemainingAssessmentsdirectly
You currently wrap theRemainingAssessmentsobject in an additional property. The spec and implementation suggest returning the detailed object itself. Please update to:- schema: - type: object - properties: - remainingAssessments: - $ref: "#/components/schemas/RemainingAssessments" + schema: + $ref: "#/components/schemas/RemainingAssessments"
🧹 Nitpick comments (6)
servers/assessment/docs/openapi.yaml (6)
271-289: Fix summary typo and clarify endpoint semantics
Thesummaryon the GET/student-assessment/{courseParticipationID}endpoint contains a typo ("fo") and is vague. Consider:
- Correcting "fo" → "for".
- Clarifying that this returns the aggregated
StudentAssessmentobject for a given course participation in a course phase.Example:
summary: Get the aggregated student assessment for a course participation in a course phase
396-406: Use a dedicated request schema for marking completion
ReusingAssessmentCompletionfor the POST request body can be confusing because it includes server-managed fields (completedAt) and thecompletedflag. Define a newCreateAssessmentCompletionRequestschema that captures only the client-supplied fields (coursePhaseID,courseParticipationID,author), and continue usingAssessmentCompletionfor responses.
428-441: Consider HTTP 204 No Content for DELETE
Your DELETE/student-assessment/completed/course-participation/{courseParticipationID}returns a 200 with only a description. By REST conventions, a 204 No Content is more appropriate when there is no response body. You may want to change:responses: "204": description: AssessmentCompletion deletedand remove the content block.
631-640: Declare required fields forRemainingAssessments
To improve client generation and runtime validation, explicitly list which properties are mandatory. For example:RemainingAssessments: type: object required: - remainingAssessments - categories properties: remainingAssessments: type: integer categories: type: array items: $ref: "#/components/schemas/CategoryWithRemainingAssessments"
641-657: AddrequiredtoAssessmentCompletionand clarify timestamp ownership
Mark key properties as required (e.g.,coursePhaseID,courseParticipationID,completedAt,completed). Also decide ifcompletedAtis client-supplied or server-generated—if server-side, move it to the response schema only. Example:AssessmentCompletion: type: object required: - coursePhaseID - courseParticipationID - completed properties: completedAt: type: string format: date-time ...
658-675: Specify required properties forStudentAssessment
For clarity and stronger typing, list the mandatory fields:StudentAssessment: type: object required: - coursePhaseID - courseParticipationID - assessments - remainingAssessments - assessmentCompletion properties: ...
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
servers/assessment/docs/openapi.yaml(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- 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 / team-allocation / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-server-team-allocation
- GitHub Check: build-servers / assessment / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-server-assessment
- GitHub Check: build-servers / core / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-server-core
- GitHub Check: build-servers / team-allocation / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-server-team-allocation
- GitHub Check: build-servers / assessment / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-server-assessment
- GitHub Check: build-servers / intro-course / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-server-intro-course
- GitHub Check: build-servers / intro-course / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-server-intro-course
🔇 Additional comments (1)
servers/assessment/docs/openapi.yaml (1)
410-427: Ensure spec matches router behavior for GET completion
The GET/student-assessment/completed/course-participation/{courseParticipationID}is specified to return the rawAssessmentCompletionobject. Verify that your handler does not wrap the response in an envelope (e.g.,{ assessmentCompletion: ... }). If it does, update the spec accordingly, or adjust the handler to return unwrapped JSON.
…status in AssessmentForm and AssessmentProfile components
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
clients/assessment_component/src/assessment/pages/AssessmentPage/components/AssessmentProfile.tsx (1)
58-88: Properly sequence mutations to prevent race conditionsThe implementation has been improved to await the completion mutation before updating the participation, which is good. However, there's still a potential race condition between the mutations and dialog closure.
const handleConfirm = () => { let passStatus = PassStatus.NOT_ASSESSED - const handleCompletion = async () => { + (async () => { try { if (studentAssessment.assessmentCompletion.completed) { await deleteCompletion(studentAssessment.courseParticipationID) } else { await createCompletion({ courseParticipationID: studentAssessment.courseParticipationID, coursePhaseID: phaseId ?? '', author: userName, completedAt: new Date().toISOString(), completed: true, }) passStatus = PassStatus.PASSED } await updateParticipation({ coursePhaseID: participant.coursePhaseID, courseParticipationID: participant.courseParticipationID, passStatus: passStatus, restrictedData: participant.restrictedData, studentReadableData: participant.studentReadableData, }) setDialogOpen(false) } catch (err) { setError('An error occurred while updating the assessment completion status.') } - } - - handleCompletion() + })() }
🧹 Nitpick comments (6)
clients/assessment_component/src/assessment/pages/AssessmentPage/components/AssessmentProfile.tsx (6)
65-67: Add validation for phaseIdThe code uses
phaseId ?? ''which could lead to silent errors if phaseId is undefined. Consider adding explicit validation.await createCompletion({ courseParticipationID: studentAssessment.courseParticipationID, - coursePhaseID: phaseId ?? '', + coursePhaseID: phaseId || (() => { throw new Error('Phase ID is required'); })(), author: userName, completedAt: new Date().toISOString(), completed: true, })
129-133: Reuse existing string capitalization utilityInstead of manually implementing string capitalization, consider using a shared utility function if one exists in the codebase. This would improve consistency and maintainability.
<span className='text-muted-foreground'> {participant.student.studyDegree - ? participant.student.studyDegree.charAt(0).toUpperCase() + - participant.student.studyDegree.slice(1) + ? capitalizeFirstLetter(participant.student.studyDegree) : 'N/A'} </span>
167-180: Improve dialog text accessibility with more contextThe dialog text could be more descriptive about the implications of marking an assessment as final or reopening it for editing. Consider adding details about what happens to the student's view or course progression.
183-188: Enhance error message with more specific informationThe current error message is generic. Consider enhancing it to provide more specific information about what went wrong and what the user can do to resolve it.
{error && ( <Alert variant='destructive'> - <AlertDescription>{error}</AlertDescription> + <AlertDescription> + {error} + <div className="mt-2 text-sm"> + Please try again or contact support if the problem persists. + </div> + </AlertDescription> </Alert> )}
144-148: Consider using a toast notification for errorsFor a better user experience, consider using a toast notification for transient errors rather than an inline alert that takes up space. This would make the UI cleaner while still informing the user about errors.
69-70: Use a more precise timestamp format for completedAtUsing
new Date().toISOString()is acceptable, but consider using a more specific date formatting library likedate-fns(which is already imported) to ensure consistent timestamp formatting across the application.await createCompletion({ courseParticipationID: studentAssessment.courseParticipationID, coursePhaseID: phaseId ?? '', author: userName, - completedAt: new Date().toISOString(), + completedAt: format(new Date(), "yyyy-MM-dd'T'HH:mm:ss.SSSxxx"), completed: true, })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
clients/assessment_component/src/assessment/pages/AssessmentPage/components/AssessmentForm.tsx(7 hunks)clients/assessment_component/src/assessment/pages/AssessmentPage/components/AssessmentProfile.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- clients/assessment_component/src/assessment/pages/AssessmentPage/components/AssessmentForm.tsx
🧰 Additional context used
🧠 Learnings (1)
clients/assessment_component/src/assessment/pages/AssessmentPage/components/AssessmentProfile.tsx (1)
Learnt from: rappm
PR: ls1intum/prompt2#415
File: clients/assessment_component/src/assessment/network/mutations/createAssessmentCompletion.ts:1-3
Timestamp: 2025-04-17T18:25:46.423Z
Learning: In this codebase, coursePhaseID and courseParticipationID parameters are already validated elsewhere in the application flow and do not require additional validation in mutation functions like createAssessmentCompletion.
🔇 Additional comments (1)
clients/assessment_component/src/assessment/pages/AssessmentPage/components/AssessmentProfile.tsx (1)
65-71: Completed flag is now correctly set to trueI see the
completedflag is now correctly set totruewhen creating a completion, which fixes the previously identified issue.
...ts/assessment_component/src/assessment/pages/AssessmentPage/components/AssessmentProfile.tsx
Show resolved
Hide resolved
...ts/assessment_component/src/assessment/pages/AssessmentPage/components/AssessmentProfile.tsx
Outdated
Show resolved
Hide resolved
...ts/assessment_component/src/assessment/pages/AssessmentPage/components/AssessmentProfile.tsx
Outdated
Show resolved
Hide resolved
|
General Comment: Your loading is not handled great. Instead of a loading, the user gets presented with an error screen if you have bad internet connection (or simulate slow internet) |
clients/assessment_component/src/assessment/pages/AssessmentPage/components/AssessmentForm.tsx
Show resolved
Hide resolved
...ts/assessment_component/src/assessment/pages/AssessmentPage/components/AssessmentProfile.tsx
Outdated
Show resolved
Hide resolved
...ts/assessment_component/src/assessment/pages/AssessmentPage/components/AssessmentProfile.tsx
Outdated
Show resolved
Hide resolved
...ts/assessment_component/src/assessment/pages/AssessmentPage/components/AssessmentProfile.tsx
Outdated
Show resolved
Hide resolved
...ts/assessment_component/src/assessment/pages/AssessmentPage/components/AssessmentProfile.tsx
Show resolved
Hide resolved
…nalization and reopening
…smentProfile component
…or better error handling
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
clients/assessment_component/src/assessment/pages/AssessmentPage/components/AssessmentProfile.tsx (1)
113-114: Confusing nested property naming.As noted in previous comments, using
remainingAssessments.remainingAssessmentscreates a confusing property chain. Consider renaming the inner property to something likecountortotalfor clarity.#!/bin/bash # Check the structure of the StudentAssessment interface rg -n "export (type|interface) StudentAssessment" -A 10 --glob "*.ts" # Check the structure of the RemainingAssessmentsForStudent type rg -n "export (type|interface) RemainingAssessmentsForStudent" -A 5 --glob "*.ts"servers/assessment/assessments/assessmentCompletion/router.go (3)
16-16:⚠️ Potential issueFix function name capitalization inconsistency.
The function name has an unconventional uppercase 'T' in the middle which is inconsistent with Go naming conventions.
- assessmentRouter.POST("", authMiddleware(promptSDK.PromptAdmin, promptSDK.CourseLecturer, promptSDK.CourseEditor), markAssessmenTAsComplete) + assessmentRouter.POST("", authMiddleware(promptSDK.PromptAdmin, promptSDK.CourseLecturer, promptSDK.CourseEditor), markAssessmentAsComplete)
21-21:⚠️ Potential issueFix function name capitalization.
The function name has an unconventional uppercase 'T' in the middle which is inconsistent with Go naming conventions.
- func markAssessmenTAsComplete(c *gin.Context) { + func markAssessmentAsComplete(c *gin.Context) {
64-64:⚠️ Potential issueFix parameter order in GetAssessmentCompletion call.
The order of parameters is swapped in the call to
GetAssessmentCompletion. Based on the service function signature from the relevant code snippets, the function expects courseParticipationID first, then coursePhaseID.- assessmentCompletion, err := GetAssessmentCompletion(c, courseParticipationID, coursePhaseID) + assessmentCompletion, err := GetAssessmentCompletion(c, courseParticipationID, coursePhaseID)
🧹 Nitpick comments (8)
clients/assessment_component/src/assessment/pages/AssessmentPage/components/AssessmentCompletionDialog.tsx (2)
69-73: Consider adding an icon to the error alert.For better UX, you might want to add an icon to the error alert to make it more noticeable.
<Alert variant='destructive'> + <AlertIcon className="h-4 w-4" /> <AlertDescription>{error}</AlertDescription> </Alert>
40-42: Clear error on dialog open change.The dialog handles error state well by clearing errors when closed, but you might want to consider also clearing errors when the dialog is opened to ensure a clean state for new interactions.
onOpenChange={(open) => { - if (!open) setError(null) // Clear error when closing dialog + setError(null) // Clear error when dialog state changes setDialogOpen(open) }}clients/assessment_component/src/assessment/pages/AssessmentPage/components/AssessmentStatusBadge.tsx (2)
18-19: Naming improvement for clarity.Consider renaming
isCompletedButNotFinalizedto simplyisReadyToFinalizeto match the UI text and improve code readability.- const isCompletedButNotFinalized = isCompleted && !isFinalized + const isReadyToFinalize = isCompleted && !isFinalized
26-27: Reference the named constant for consistency in conditional styling.Use the named constant
isReadyToFinalizein the conditional styling for consistency with the render logic.'bg-blue-100 text-blue-800 dark:bg-blue-900/30 dark:text-blue-400': - isCompletedButNotFinalized, + isReadyToFinalize,clients/assessment_component/src/assessment/pages/AssessmentPage/components/AssessmentProfile.tsx (1)
150-154: Consider adding a dismiss button to the error alert.For better user experience, you might want to add a dismiss button to the error alert to allow users to clear errors manually.
- <Alert variant='destructive' className='mt-4'> + <Alert variant='destructive' className='mt-4' + action={<Button variant="ghost" size="sm" onClick={() => setError(null)}>Dismiss</Button>}> <AlertDescription>{error}</AlertDescription> </Alert>servers/assessment/validation/assessmentCompletion.go (1)
12-26: The validation function is well-implemented for checking assessment completion status.The function correctly checks if an assessment completion record exists for a given course participation and course phase, and prevents modifications to completed assessments by returning an appropriate error message.
Consider enhancing the error messages to include more context such as the IDs being checked, which would make debugging easier. This would be especially helpful in production environments where tracking down specific error cases can be challenging.
- log.Error("could not check assessment completion existence: ", err) - return errors.New("could not check assessment completion existence") + log.WithFields(log.Fields{ + "courseParticipationID": courseParticipationID, + "coursePhaseID": coursePhaseID, + }).Error("could not check assessment completion existence: ", err) + return fmt.Errorf("could not check assessment completion existence for participation %s and phase %s: %w", + courseParticipationID, coursePhaseID, err)- log.Error("cannot create/update assessment, completion already exists") - return errors.New("assessment was already marked as completed and cannot be modified") + log.WithFields(log.Fields{ + "courseParticipationID": courseParticipationID, + "coursePhaseID": coursePhaseID, + }).Error("cannot create/update assessment, completion already exists") + return fmt.Errorf("assessment for participation %s and phase %s was already marked as completed and cannot be modified", + courseParticipationID, coursePhaseID)Note: You'll need to add
"fmt"to your imports if you make this change.servers/assessment/assessments/assessmentCompletion/router.go (2)
32-32: Update success message to match the operation.The success message says "Assessment created successfully" but the operation is actually marking an assessment as completed, not creating a new assessment.
- c.JSON(http.StatusCreated, gin.H{"message": "Assessment created successfully"}) + c.JSON(http.StatusCreated, gin.H{"message": "Assessment marked as completed successfully"})
1-75: Add loading state management to improve UX.Based on the PR comments, there's an issue with loading states not being properly managed, causing users to see error screens rather than loading indicators during slow/poor internet connections. Consider implementing loading state management in the frontend components that interact with these API endpoints.
Would you like me to suggest implementations for loading state management in the frontend components that consume these API endpoints?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
clients/assessment_component/src/assessment/pages/AssessmentPage/AssessmentPage.tsx(3 hunks)clients/assessment_component/src/assessment/pages/AssessmentPage/components/AssessmentCompletionDialog.tsx(1 hunks)clients/assessment_component/src/assessment/pages/AssessmentPage/components/AssessmentProfile.tsx(1 hunks)clients/assessment_component/src/assessment/pages/AssessmentPage/components/AssessmentStatusBadge.tsx(1 hunks)servers/assessment/assessments/assessmentCompletion/router.go(1 hunks)servers/assessment/assessments/service.go(4 hunks)servers/assessment/validation/assessmentCompletion.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- clients/assessment_component/src/assessment/pages/AssessmentPage/AssessmentPage.tsx
- servers/assessment/assessments/service.go
🧰 Additional context used
🧠 Learnings (3)
servers/assessment/validation/assessmentCompletion.go (1)
Learnt from: rappm
PR: ls1intum/prompt2#415
File: clients/assessment_component/src/assessment/network/mutations/createAssessmentCompletion.ts:1-3
Timestamp: 2025-04-17T18:25:46.423Z
Learning: In this codebase, coursePhaseID and courseParticipationID parameters are already validated elsewhere in the application flow and do not require additional validation in mutation functions like createAssessmentCompletion.
servers/assessment/assessments/assessmentCompletion/router.go (2)
Learnt from: rappm
PR: ls1intum/prompt2#415
File: servers/assessment/assessments/scoreLevel/router.go:0-0
Timestamp: 2025-04-17T19:34:33.988Z
Learning: In the assessment server (and other components like intro_course and team_allocation), the coursePhaseID parameter is already included in the base router path defined at a higher level (typically in main.go as something like api := router.Group("assessment/api/course_phase/:coursePhaseID")). This means that subroutes don't need to and shouldn't explicitly include the coursePhaseID in their paths, as it's automatically available to all handlers through c.Param("coursePhaseID").
Learnt from: rappm
PR: ls1intum/prompt2#344
File: servers/assessment/assessments/router.go:20-20
Timestamp: 2025-04-01T12:17:59.359Z
Learning: In the assessment server, the coursePhaseID parameter is already included in the base router path from the main.go file (api := router.Group("assessment/api/course_phase/:coursePhaseID")), so routes defined in module routers don't need to explicitly include it in their paths.
clients/assessment_component/src/assessment/pages/AssessmentPage/components/AssessmentProfile.tsx (2)
Learnt from: rappm
PR: ls1intum/prompt2#415
File: clients/assessment_component/src/assessment/network/mutations/createAssessmentCompletion.ts:1-3
Timestamp: 2025-04-17T18:25:46.423Z
Learning: In this codebase, coursePhaseID and courseParticipationID parameters are already validated elsewhere in the application flow and do not require additional validation in mutation functions like createAssessmentCompletion.
Learnt from: niclasheun
PR: ls1intum/prompt2#411
File: clients/assessment_component/src/assessment/pages/AssessmentPage/components/AssessmentProfile.tsx:40-43
Timestamp: 2025-04-17T22:48:27.053Z
Learning: When implementing loading states with React Query's mutation hooks, use the built-in `isPending` property instead of creating a separate state variable. Multiple loading states can be combined with logical OR (||) to create a unified loading state.
🧬 Code Graph Analysis (3)
clients/assessment_component/src/assessment/pages/AssessmentPage/components/AssessmentStatusBadge.tsx (1)
clients/shared_library/lib/utils.ts (1)
cn(4-6)
servers/assessment/validation/assessmentCompletion.go (1)
servers/assessment/db/sqlc/assessment_completion.sql.go (1)
CheckAssessmentCompletionExistsParams(24-27)
servers/assessment/assessments/assessmentCompletion/router.go (3)
clients/assessment_component/src/assessment/interfaces/assessment.ts (1)
AssessmentCompletion(34-40)servers/assessment/assessments/assessmentCompletion/assessmentCompletionDTO/assessmentCompletion.go (1)
AssessmentCompletion(9-15)servers/assessment/assessments/assessmentCompletion/service.go (3)
MarkAssessmentAsCompleted(38-74)UnmarkAssessmentAsCompleted(76-86)GetAssessmentCompletion(97-107)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- 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 / team-allocation / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-server-team-allocation
- GitHub Check: build-servers / assessment / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-server-assessment
- GitHub Check: build-servers / assessment / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-server-assessment
- GitHub Check: build-servers / team-allocation / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-server-team-allocation
- GitHub Check: build-servers / core / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-server-core
- 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 / intro-course / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-server-intro-course
🔇 Additional comments (5)
clients/assessment_component/src/assessment/pages/AssessmentPage/components/AssessmentCompletionDialog.tsx (1)
1-94: Well-structured dialog component with comprehensive state handling.This new dialog component provides a clear UI for confirming assessment finalization status. The component effectively:
- Adapts its content based on completion state
- Handles loading states with the
isPendingprop- Clears errors when closing the dialog
- Shows meaningful action buttons with appropriate styling
clients/assessment_component/src/assessment/pages/AssessmentPage/components/AssessmentStatusBadge.tsx (1)
1-19: Good enhancement of status badge with finalized state visualization.The introduction of the
isFinalizedprop and related logic provides clear visual differentiation between completed assessments that are finalized vs. those ready to be finalized. The component now has three distinct states with appropriate styling and icons.clients/assessment_component/src/assessment/pages/AssessmentPage/components/AssessmentProfile.tsx (2)
55-85: Good job fixing the race condition in the handleConfirm function.You've properly addressed the previous race condition by awaiting the completion of mutations before closing the dialog. This ensures that errors can be properly displayed to users.
89-167: Well-structured UI with appropriate loading states and error handling.The UI has been well-designed with:
- A clear button to initiate finalization actions
- Visual indicators (lock/unlock icons) to represent assessment state
- Proper error display
- Integration with the AssessmentCompletionDialog
- Good handling of loading states using the isPending flags
servers/assessment/assessments/assessmentCompletion/router.go (1)
17-18:⚠️ Potential issueUpdate route paths to include coursePhaseID parameter.
The GET and DELETE routes don't include the coursePhaseID parameter in the path, but the handlers try to extract it using
c.Param("coursePhaseID"). Based on the implementations ofgetAssessmentCompletionandunmarkAssessmentAsCompleted, both coursePhaseID and courseParticipationID are required.- assessmentRouter.GET("/course-participation/:courseParticipationID", authMiddleware(promptSDK.PromptAdmin, promptSDK.CourseLecturer, promptSDK.CourseEditor), getAssessmentCompletion) - assessmentRouter.DELETE("/course-participation/:courseParticipationID", authMiddleware(promptSDK.PromptAdmin, promptSDK.CourseLecturer, promptSDK.CourseEditor), unmarkAssessmentAsCompleted) + assessmentRouter.GET("/course-participation/:courseParticipationID/course-phase/:coursePhaseID", authMiddleware(promptSDK.PromptAdmin, promptSDK.CourseLecturer, promptSDK.CourseEditor), getAssessmentCompletion) + assessmentRouter.DELETE("/course-participation/:courseParticipationID/course-phase/:coursePhaseID", authMiddleware(promptSDK.PromptAdmin, promptSDK.CourseLecturer, promptSDK.CourseEditor), unmarkAssessmentAsCompleted)⛔ Skipped due to learnings
Learnt from: rappm PR: ls1intum/prompt2#415 File: servers/assessment/assessments/scoreLevel/router.go:0-0 Timestamp: 2025-04-17T19:34:33.988Z Learning: In the assessment server (and other components like intro_course and team_allocation), the coursePhaseID parameter is already included in the base router path defined at a higher level (typically in main.go as something like api := router.Group("assessment/api/course_phase/:coursePhaseID")). This means that subroutes don't need to and shouldn't explicitly include the coursePhaseID in their paths, as it's automatically available to all handlers through c.Param("coursePhaseID").Learnt from: rappm PR: ls1intum/prompt2#344 File: servers/assessment/assessments/router.go:20-20 Timestamp: 2025-04-01T12:17:59.359Z Learning: In the assessment server, the coursePhaseID parameter is already included in the base router path from the main.go file (api := router.Group("assessment/api/course_phase/:coursePhaseID")), so routes defined in module routers don't need to explicitly include it in their paths.Learnt from: mathildeshagl PR: ls1intum/prompt2#406 File: servers/team_allocation/allocations/router.go:69-89 Timestamp: 2025-04-15T16:42:55.313Z Learning: In the team_allocation component (like intro_course and assessment), the coursePhaseID parameter is already included in the base router path from the main.go file, so routes defined in module routers don't need to explicitly include it in their paths.Learnt from: mathildeshagl PR: ls1intum/prompt2#406 File: servers/team_allocation/allocations/router.go:53-67 Timestamp: 2025-04-15T16:44:04.015Z Learning: In the team_allocation component, similar to other components like intro_course and assessment, the coursePhaseID parameter is already included in the base router path defined in main.go (api := router.Group("team-allocation/api/course_phase/:coursePhaseID")), so routes defined in module routers don't need to explicitly include it in their paths.Learnt from: niclasheun PR: ls1intum/prompt2#305 File: servers/intro_course/infrastructureSetup/router.go:27-49 Timestamp: 2025-03-22T12:38:08.069Z Learning: In the intro_course project, the coursePhaseID parameter is already included in the base router path from the main.go file (api := router.Group("intro-course/api/course_phase/:coursePhaseID")), so routes defined in module routers don't need to explicitly include it in their paths.Learnt from: mathildeshagl PR: ls1intum/prompt2#406 File: servers/team_allocation/allocations/router.go:53-67 Timestamp: 2025-04-15T16:44:04.015Z Learning: In the team_allocation component, similar to other components like intro_course and assessment, the coursePhaseID parameter is already included in the base router path defined in main.go, so routes defined in module routers don't need to explicitly include it in their paths.Learnt from: niclasheun PR: ls1intum/prompt2#305 File: servers/intro_course/infrastructureSetup/router.go:27-49 Timestamp: 2025-03-22T12:38:08.069Z Learning: In the intro_course project, the coursePhaseID parameter is already included in the base router path from the main.go file, so routes defined in module routers don't need to explicitly include it in their paths.Learnt from: rappm PR: ls1intum/prompt2#392 File: servers/assessment/assessments/router.go:176-195 Timestamp: 2025-04-10T14:41:59.900Z Learning: In the assessment server implementation using Gin, the coursePhaseID parameter is declared at the router group level, making it accessible to all child routes without needing to redeclare it in each subendpoint handler.
...ts/assessment_component/src/assessment/pages/AssessmentPage/components/AssessmentProfile.tsx
Outdated
Show resolved
Hide resolved
...ts/assessment_component/src/assessment/pages/AssessmentPage/components/AssessmentProfile.tsx
Outdated
Show resolved
Hide resolved
…sable button based on remaining assessments
|
Done @niclasheun |
|
LGTM |
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores