Assessment: introduce deadline for assessment phase
#607
Conversation
|
""" WalkthroughA deadline feature for assessment phases was introduced across both frontend and backend. Backend changes include new database schema, SQL queries, DTOs, service, and HTTP endpoints for managing deadlines. Frontend changes implement React hooks, Zustand store, network mutations/queries, UI components, and page integration to fetch, display, and update deadlines for course phases. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SettingsPage
participant DeadlineSelection
participant useUpdateDeadline
participant updateDeadline (API)
participant Backend (Gin Handler)
participant Service
participant DB
User->>SettingsPage: Navigates to settings
SettingsPage->>DeadlineSelection: Renders
DeadlineSelection->>useUpdateDeadline: User selects date & clicks update
useUpdateDeadline->>updateDeadline (API): PUT /course_phase/{id}/deadline
updateDeadline (API)->>Backend (Gin Handler): Receives request
Backend (Gin Handler)->>Service: Calls UpdateCoursePhaseDeadline
Service->>DB: Updates deadline
DB-->>Service: Success/Failure
Service-->>Backend (Gin Handler): Result
Backend (Gin Handler)-->>updateDeadline (API): HTTP 201/500
updateDeadline (API)-->>useUpdateDeadline: Resolves Promise
useUpdateDeadline-->>DeadlineSelection: Updates state/UI
sequenceDiagram
participant AssessmentPage
participant useGetDeadline
participant getDeadline (API)
participant Backend (Gin Handler)
participant Service
participant DB
AssessmentPage->>useGetDeadline: On mount
useGetDeadline->>getDeadline (API): GET /course_phase/{id}/deadline
getDeadline (API)->>Backend (Gin Handler): Receives request
Backend (Gin Handler)->>Service: Calls GetCoursePhaseDeadline
Service->>DB: Fetches deadline
DB-->>Service: Returns deadline
Service-->>Backend (Gin Handler): Deadline or nil
Backend (Gin Handler)-->>getDeadline (API): HTTP 200 with deadline
getDeadline (API)-->>useGetDeadline: Returns Date
useGetDeadline-->>AssessmentPage: Sets deadline in store/UI
Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 1
🧹 Nitpick comments (5)
clients/assessment_component/src/assessment/zustand/useDeadlineStore.tsx (1)
3-6: Consider supporting deadline clearing in the setter function.The
deadlinestate can beundefined, butsetDeadlineonly acceptsDate. Consider allowingundefinedin the setter to enable clearing the deadline.export interface DeadlineStore { deadline: Date | undefined - setDeadline: (deadline: Date) => void + setDeadline: (deadline: Date | undefined) => void }clients/assessment_component/src/assessment/pages/AssessmentPage/components/AssessmentCompletion/AssessmentCompletion.tsx (1)
232-236: Simplify redundant conditional logicThe deadline display has redundant conditional checks. The outer condition already verifies
deadlineexists, making the inner ternary unnecessary.- {deadline && ( - <div className='text-muted-foreground'> - Deadline: {deadline ? format(new Date(deadline), 'dd.MM.yyyy') : 'No deadline set'} - </div> - )} + {deadline && ( + <div className='text-muted-foreground'> + Deadline: {format(new Date(deadline), 'dd.MM.yyyy')} + </div> + )}clients/assessment_component/src/assessment/pages/AssessmentDataShell.tsx (1)
94-98: Simplify the redundant null check condition.The condition
deadline && deadline != nullis redundant since the&&operator already handles falsy values includingnull.- useEffect(() => { - if (deadline && deadline != null) { - setDeadline(deadline) - } - }, [deadline, setDeadline]) + useEffect(() => { + if (deadline) { + setDeadline(deadline) + } + }, [deadline, setDeadline])clients/assessment_component/src/assessment/pages/SettingsPage/components/DeadlineSelection/DeadlineSelection.tsx (1)
50-64: Consider simplifying the date handling in DatePicker.The date formatting in the
onSelecthandler might be unnecessary sinceDatePickershould already handleDateobjects properly.- <DatePicker - date={deadline} - onSelect={(date) => - setDeadline(date ? new Date(format(date, 'yyyy-MM-dd')) : undefined) - } - /> + <DatePicker + date={deadline} + onSelect={(date) => setDeadline(date || undefined)} + />If the current formatting is required for specific DatePicker behavior, please keep the current implementation.
servers/assessment/coursePhaseConfig/router.go (1)
38-61: Consider using HTTP 200 OK instead of 201 Created for updates.The PUT handler logic is sound, but line 60 returns
http.StatusCreated(201) for an update operation. HTTP 200 OK or 204 No Content would be more semantically correct for updates, as 201 Created is typically reserved for resource creation.- c.Status(http.StatusCreated) + c.Status(http.StatusOK)The rest of the handler properly validates input, handles errors, and maintains consistent logging patterns.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
clients/assessment_component/src/assessment/network/mutations/updateDeadline.ts(1 hunks)clients/assessment_component/src/assessment/network/queries/getDeadline.ts(1 hunks)clients/assessment_component/src/assessment/pages/AssessmentDataShell.tsx(4 hunks)clients/assessment_component/src/assessment/pages/AssessmentPage/AssessmentPage.tsx(2 hunks)clients/assessment_component/src/assessment/pages/AssessmentPage/components/AssessmentCompletion/AssessmentCompletion.tsx(3 hunks)clients/assessment_component/src/assessment/pages/SettingsPage/SettingsPage.tsx(2 hunks)clients/assessment_component/src/assessment/pages/SettingsPage/components/AssessmentTemplateSelection/AssessmentTemplateSelection.tsx(1 hunks)clients/assessment_component/src/assessment/pages/SettingsPage/components/DeadlineSelection/DeadlineSelection.tsx(1 hunks)clients/assessment_component/src/assessment/pages/SettingsPage/components/DeadlineSelection/hooks/useUpdateDeadline.ts(1 hunks)clients/assessment_component/src/assessment/pages/hooks/useGetDeadline.ts(1 hunks)clients/assessment_component/src/assessment/zustand/useDeadlineStore.tsx(1 hunks)servers/assessment/coursePhaseConfig/coursePhaseConfigDTO/deadline.go(1 hunks)servers/assessment/coursePhaseConfig/main.go(1 hunks)servers/assessment/coursePhaseConfig/router.go(1 hunks)servers/assessment/coursePhaseConfig/service.go(1 hunks)servers/assessment/db/migration/0012_add_assessment_deadline.up.sql(1 hunks)servers/assessment/db/query/assessment_template.sql(1 hunks)servers/assessment/db/query/category.sql(2 hunks)servers/assessment/db/query/course_phase_config.sql(1 hunks)servers/assessment/db/sqlc/assessment_template.sql.go(0 hunks)servers/assessment/db/sqlc/category.sql.go(2 hunks)servers/assessment/db/sqlc/course_phase_config.sql.go(1 hunks)servers/assessment/db/sqlc/models.go(1 hunks)servers/assessment/main.go(2 hunks)
💤 Files with no reviewable changes (1)
- servers/assessment/db/sqlc/assessment_template.sql.go
🧰 Additional context used
🧠 Learnings (3)
servers/assessment/main.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#415
File: servers/assessment/assessments/scoreLevel/router.go:0-0
Timestamp: 2025-04-17T19:34:34.071Z
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").
servers/assessment/coursePhaseConfig/main.go (4)
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: 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: 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.
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.
servers/assessment/coursePhaseConfig/router.go (5)
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.
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: rappm
PR: ls1intum/prompt2#415
File: servers/assessment/assessments/scoreLevel/router.go:0-0
Timestamp: 2025-04-17T19:34:34.071Z
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: 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.
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: test-servers / test (assessment)
- GitHub Check: lint-clients / lint-client (self_team_allocation_component)
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (41)
clients/assessment_component/src/assessment/pages/AssessmentPage/AssessmentPage.tsx (3)
4-4: LGTM: Import update for consistent loading UIThe replacement of
Loader2withLoadingPagefrom the UI components library promotes consistency across the application.
38-38: LGTM: Simplified loading state renderingThe replacement of custom spinner JSX with the standardized
<LoadingPage />component improves code consistency and maintainability.
74-77: LGTM: Deadline prop removal aligns with new architectureThe removal of the hardcoded
deadlineprop fromAssessmentCompletioncorrectly aligns with the new centralized deadline state management using Zustand store, as mentioned in the AI summary.clients/assessment_component/src/assessment/pages/SettingsPage/SettingsPage.tsx (2)
3-3: LGTM: Clean import additionThe import of
DeadlineSelectioncomponent is correctly added and follows the existing import structure.
12-16: LGTM: Well-structured responsive grid layoutThe grid layout implementation is clean and provides a good responsive design with appropriate spacing. The use of
xl:grid-cols-3withgap-4creates a well-organized settings interface.servers/assessment/db/query/category.sql (2)
4-6: LGTM: Consistent table name update in CREATE queryThe subquery correctly references the new
course_phase_configtable instead of the oldassessment_template_course_phasetable, maintaining the same filtering logic.
24-26: LGTM: Consistent table name update in UPDATE queryThe subquery update is consistent with the CREATE query, properly referencing the new
course_phase_configtable while preserving the original filtering logic.clients/assessment_component/src/assessment/network/queries/getDeadline.ts (1)
3-13: ```shell
#!/bin/bashSearch for the assessmentAxiosInstance definition across the repository
rg -n "assessmentAxiosInstance" -C 5
</details> <details> <summary>servers/assessment/main.go (2)</summary> `20-20`: **LGTM: Clean import addition** The import of the `coursePhaseConfig` package follows Go conventions and aligns with the existing import structure. --- `96-96`: **LGTM: Consistent module initialization** The initialization of the course phase configuration module follows the established pattern used by other modules and is properly placed in the initialization sequence. The coursePhaseID parameter is already available from the base router path as expected. </details> <details> <summary>servers/assessment/db/sqlc/models.go (1)</summary> `133-137`: **LGTM! Model structure aligns with database schema changes.** The new `CoursePhaseConfig` struct correctly represents the updated database table with the addition of the `Deadline` field using the appropriate `pgtype.Timestamptz` type for nullable timestamps. </details> <details> <summary>servers/assessment/db/sqlc/category.sql.go (2)</summary> `19-19`: **LGTM! Table reference updated correctly.** The SQL query correctly references the new `course_phase_config` table, aligning with the database schema changes. --- `196-196`: **LGTM! Consistent table reference update.** The update query correctly uses the new `course_phase_config` table name, maintaining consistency with the schema changes. </details> <details> <summary>clients/assessment_component/src/assessment/pages/SettingsPage/components/AssessmentTemplateSelection/AssessmentTemplateSelection.tsx (2)</summary> `45-45`: **LGTM! Grid layout adjustment supports the new deadline feature.** The `xl:col-span-2` class appropriately adjusts the component's layout to accommodate the new deadline selection component in the settings page. --- `49-49`: **LGTM! Title shortened for better UI balance.** The shortened title "Template" works well within the updated layout constraints. </details> <details> <summary>clients/assessment_component/src/assessment/pages/AssessmentPage/components/AssessmentCompletion/AssessmentCompletion.tsx (1)</summary> `5-5`: **LGTM: Clean integration with deadline store** The imports and store integration are properly implemented. The component now correctly retrieves the deadline from the centralized Zustand store. Also applies to: 32-32, 45-45 </details> <details> <summary>clients/assessment_component/src/assessment/network/mutations/updateDeadline.ts (1)</summary> `1-18`: **LGTM: Well-structured API mutation function** The implementation follows good practices with proper TypeScript typing, appropriate HTTP method, and consistent error handling pattern. The function correctly uses the pre-configured axios instance and includes proper content-type headers. </details> <details> <summary>servers/assessment/coursePhaseConfig/coursePhaseConfigDTO/deadline.go (1)</summary> `1-13`: **LGTM: Well-designed DTOs with proper nullability handling** The DTO design correctly uses `time.Time` for the required request field and `*time.Time` for the nullable response field. JSON tags are properly configured for API serialization. </details> <details> <summary>servers/assessment/db/migration/0012_add_assessment_deadline.up.sql (1)</summary> `1-16`: **LGTM: Well-structured database migration** The migration properly: - Uses transaction boundaries for atomicity - Renames the table to better reflect its purpose - Adds nullable `deadline` column with appropriate `TIMESTAMPTZ` type for timezone support - Updates the dependent view to maintain consistency The nullable deadline column with `DEFAULT NULL` correctly allows existing records to have no deadline set. </details> <details> <summary>servers/assessment/db/query/assessment_template.sql (1)</summary> `27-30`: **LGTM: Consistent cleanup of deprecated queries** The removal of queries related to the old `assessment_template_course_phase` table structure is consistent with the migration to `course_phase_config`. The remaining query maintains the essential assessment template functionality. </details> <details> <summary>clients/assessment_component/src/assessment/pages/AssessmentDataShell.tsx (4)</summary> `7-7`: **LGTM! Clean integration of deadline dependencies.** The deadline-related imports and store usage follow the established patterns in the component. Also applies to: 12-12, 17-17, 28-28 --- `54-67`: **LGTM! Consistent query pattern integration.** The deadline query integration follows the same pattern as other data queries, with proper error and loading state handling. --- `73-73`: **LGTM! Consistent refetch integration.** Adding deadline refetch to the refetch function maintains consistency with other data queries. --- `101-101`: **LGTM! Improved loading UI consistency.** Replacing the custom spinner with `LoadingPage` from the UI components library improves consistency and maintainability. </details> <details> <summary>servers/assessment/coursePhaseConfig/main.go (1)</summary> `1-16`: **LGTM! Clean module initialization following established patterns.** The module initialization correctly sets up routing with authentication middleware and creates the service singleton. The implementation follows the established patterns from the retrieved learnings where the coursePhaseID parameter is handled at the router group level. </details> <details> <summary>clients/assessment_component/src/assessment/pages/SettingsPage/components/DeadlineSelection/hooks/useUpdateDeadline.ts (1)</summary> `1-26`: **LGTM! Well-structured React Query mutation hook.** The hook properly handles: - Mutation with phaseId from URL params - Query invalidation on success to keep data fresh - Comprehensive error handling with server error extraction - Proper TypeScript typing The implementation follows React Query best practices. </details> <details> <summary>servers/assessment/coursePhaseConfig/service.go (3)</summary> `15-21`: **LGTM! Clean service structure with singleton pattern.** The service struct and singleton pattern are consistent with the codebase architecture. --- `22-32`: **LGTM! Proper nullable timestamp handling.** The function correctly uses `pgtype.Timestamptz` to handle nullable timestamp values in the database. --- `34-49`: **LGTM! Robust error handling for deadline retrieval.** The function properly handles the case where no deadline exists by checking for `sql.ErrNoRows` and returning `nil`, while also handling other database errors appropriately. </details> <details> <summary>clients/assessment_component/src/assessment/pages/SettingsPage/components/DeadlineSelection/DeadlineSelection.tsx (2)</summary> `19-36`: **LGTM! Well-structured state management and update logic.** The component properly synchronizes local state with the global store and handles the deadline update flow correctly. --- `67-84`: **LGTM! Excellent user feedback and information display.** The component provides clear information about the current deadline, explains the implications of setting a deadline, and properly displays error/success states. </details> <details> <summary>servers/assessment/coursePhaseConfig/router.go (2)</summary> `13-18`: **LGTM - Router setup follows established patterns.** The router configuration correctly leverages the coursePhaseID parameter from the base router path (as confirmed by retrieved learnings) and properly applies authentication middleware with appropriate roles. --- `20-36`: **LGTM - Proper error handling and response structure.** The GET handler correctly: - Validates UUID format with appropriate error response - Uses context for database operations - Returns structured JSON responses - Implements proper error logging with context </details> <details> <summary>servers/assessment/db/query/course_phase_config.sql (7)</summary> `1-5`: **LGTM - Proper upsert implementation with conflict resolution.** The upsert query correctly handles the ON CONFLICT scenario by updating the assessment_template_id when a course_phase_id already exists, which maintains the unique constraint while allowing updates. --- `7-11`: **LGTM - Standard delete operation with proper conditions.** The delete query correctly uses both IDs to ensure precise record removal, preventing accidental bulk deletions. --- `13-17`: **LGTM - Efficient join query with proper aliasing.** The query efficiently retrieves assessment template details using an inner join, with clear table aliases for readability. --- `19-22`: **LGTM - Simple and efficient selection query.** Clean query to retrieve course phase IDs for a given assessment template. --- `24-27`: **LGTM - Comprehensive listing with proper ordering.** The query includes all fields and uses logical ordering by both assessment_template_id and course_phase_id for consistent results. --- `29-32`: **LGTM - Targeted deadline update operation.** Simple and focused update query for the deadline field using the course_phase_id as the key. --- `34-37`: **LGTM - Straightforward deadline retrieval.** Clean query to fetch the deadline for a specific course phase. </details> <details> <summary>servers/assessment/db/sqlc/course_phase_config.sql.go (1)</summary> `1-149`: **LGTM - Well-generated database access layer.** The generated code correctly implements: - Proper type safety with UUID and pgtype.Timestamptz - Consistent error handling patterns - Appropriate parameter structures - Clean separation between query execution and result processing - Proper resource management with row.Close() in multi-row queries The code generation appears to have handled the SQL queries correctly, producing idiomatic Go database access methods. </details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
clients/assessment_component/src/assessment/pages/hooks/useGetDeadline.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
servers/assessment/database_dumps/categories.sql (2)
81-85: Add an index on assessment_template_id for better join performance
The newcourse_phase_configtable will frequently be joined onassessment_template_id. Without an index, those lookups will trigger full table scans. Consider adding something like:CREATE INDEX idx_course_phase_config_assessment_template_id ON public.course_phase_config (assessment_template_id);
108-109: Sample inserts omitdeadlinevalues
Currently, insertedcourse_phase_configrecords rely on the defaultNULLdeadline. If you want to exercise deadline-related behavior in development or testing, consider supplying representative non-null timestamps here. Otherwise, please confirm that defaulting toNULLis the intended approach.servers/assessment/assessments/assessmentCompletion/service.go (1)
132-143: Let’s verify thatCoursePhaseConfigSingletonis properly initialized (and not left nil) in your startup code:#!/bin/bash # Search for where the singleton is assigned rg -n "CoursePhaseConfigSingleton\\s*=" --type go servers/assessmentservers/assessment/database_dumps/coursePhaseConfig.sql (1)
51-53: Consider uncommenting sample data for better test coverage.The commented sample data could be useful for tests. Consider uncommenting it or adding it conditionally based on the test environment.
--- Sample data can be inserted here if needed for tests --- INSERT INTO public.course_phase_config (assessment_template_id, course_phase_id, deadline) VALUES --- ('550e8400-e29b-41d4-a716-446655440000', '123e4567-e89b-12d3-a456-426614174000', '2025-12-31 23:59:59+00'); +-- Sample data for testing +INSERT INTO public.course_phase_config (assessment_template_id, course_phase_id, deadline) VALUES +('550e8400-e29b-41d4-a716-446655440000', '123e4567-e89b-12d3-a456-426614174000', '2025-12-31 23:59:59+00');servers/assessment/coursePhaseConfig/router_test.go (1)
198-212: Consider more deterministic behavior for empty body handling.While the flexible assertion acknowledges uncertainty in service validation, consider defining explicit behavior for empty request bodies to improve API predictability. The current approach is acceptable but could be refined.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
clients/assessment_component/src/assessment/pages/AssessmentPage/components/AssessmentCompletion/AssessmentCompletion.tsx(5 hunks)servers/assessment/assessments/actionItem/service_test.go(1 hunks)servers/assessment/assessments/assessmentCompletion/router.go(1 hunks)servers/assessment/assessments/assessmentCompletion/router_test.go(2 hunks)servers/assessment/assessments/assessmentCompletion/service.go(2 hunks)servers/assessment/assessments/assessmentCompletion/service_test.go(2 hunks)servers/assessment/coursePhaseConfig/router_test.go(1 hunks)servers/assessment/coursePhaseConfig/service.go(1 hunks)servers/assessment/coursePhaseConfig/service_test.go(1 hunks)servers/assessment/database_dumps/actionItem.sql(1 hunks)servers/assessment/database_dumps/assessmentCompletions.sql(3 hunks)servers/assessment/database_dumps/categories.sql(3 hunks)servers/assessment/database_dumps/coursePhaseConfig.sql(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- servers/assessment/assessments/assessmentCompletion/router_test.go
- servers/assessment/database_dumps/actionItem.sql
🚧 Files skipped from review as they are similar to previous changes (2)
- clients/assessment_component/src/assessment/pages/AssessmentPage/components/AssessmentCompletion/AssessmentCompletion.tsx
- servers/assessment/coursePhaseConfig/service.go
🧰 Additional context used
🧠 Learnings (1)
servers/assessment/database_dumps/categories.sql (1)
Learnt from: rappm
PR: ls1intum/prompt2#584
File: servers/assessment/db/query/category.sql:2-6
Timestamp: 2025-06-11T13:06:10.593Z
Learning: In the `assessment_template_course_phase` table, `course_phase_id` is defined as the primary key, guaranteeing uniqueness across rows.
🪛 golangci-lint (1.64.8)
servers/assessment/assessments/actionItem/service_test.go
98-98: Error return value is not checked
(errcheck)
109-109: Error return value is not checked
(errcheck)
121-121: Error return value is not checked
(errcheck)
94-94: ineffectual assignment to err
(ineffassign)
🪛 GitHub Check: lint-servers / lint-server (servers/assessment)
servers/assessment/assessments/actionItem/service_test.go
[failure] 94-94:
ineffectual assignment to err (ineffassign)
[failure] 109-109:
Error return value is not checked (errcheck)
[failure] 98-98:
Error return value is not checked (errcheck)
servers/assessment/coursePhaseConfig/service_test.go
[failure] 87-87:
Error return value is not checked (errcheck)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- 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: Analyze (go)
🔇 Additional comments (32)
servers/assessment/database_dumps/categories.sql (1)
257-260: Confirm whether the view should expose thedeadlinecolumn
Thecategory_course_phaseview has been correctly updated to joincourse_phase_config. However, it doesn’t select the newdeadlinefield. If downstream logic needs that timestamp, you may want to addcpc.deadlineto the view’sSELECT.servers/assessment/assessments/actionItem/service_test.go (9)
1-13: LGTM! Well-structured imports and package declaration.The imports are appropriate for the test functionality, including testify for test framework, UUID generation, and local packages for DTOs and test utilities.
15-48: Good test suite structure with proper setup and cleanup.The test suite follows testify best practices with proper database setup from SQL dump and resource cleanup. The use of UUIDs for test data is appropriate.
50-61: LGTM! Clean and straightforward create test.The test properly validates action item creation with appropriate error checking.
63-70: Good edge case testing for non-existent items.Testing the error case when trying to retrieve a non-existent action item is important for robustness.
125-159: Excellent comprehensive test for listing by course phase.This test properly creates multiple action items and verifies the filtering functionality works correctly. The assertions are comprehensive and appropriate.
161-203: Well-designed test for student-specific filtering.Good test design that creates items for different students to properly validate the filtering logic. The verification ensures both student ID and course phase ID match expectations.
205-241: Good test coverage for counting functionality.The test properly validates the count functionality with multiple action items and appropriate assertions.
243-251: Excellent edge case testing for empty results.Testing the count functionality when no action items exist is important for ensuring the service handles empty results correctly.
253-255: Standard testify suite runner - LGTM!Proper integration with Go's testing framework using testify suite.
servers/assessment/assessments/assessmentCompletion/service_test.go (2)
14-14: LGTM: Necessary import for deadline functionality.The import of the coursePhaseConfig package is required for the deadline enforcement feature.
39-40: LGTM: Proper singleton initialization for tests.Initializing the CoursePhaseConfigSingleton in the test setup prevents nil pointer dereferences when the assessment completion service checks deadlines during tests. This is a good practice for dependency management in tests.
servers/assessment/assessments/assessmentCompletion/service.go (1)
15-15: LGTM: Required import for deadline functionality.The import is necessary for accessing the course phase deadline functionality.
servers/assessment/database_dumps/assessmentCompletions.sql (3)
94-99: LGTM: Well-structured table rename and deadline addition.The schema changes are properly implemented:
- Table rename from
assessment_template_course_phasetocourse_phase_configis clean and consistent- The
deadlinecolumn usestimestamp with time zonewhich properly handles timezone information- Nullable deadline allows flexibility for phases without deadlines
- Foreign key constraint is properly maintained
237-242: LGTM: Sample data updated for new table structure.The sample data inserts have been correctly updated to use the new
course_phase_configtable name.
407-410: LGTM: View and constraints properly updated.The
category_course_phaseview and foreign key constraints have been correctly updated to reference the newcourse_phase_configtable name.Also applies to: 417-418
servers/assessment/database_dumps/coursePhaseConfig.sql (3)
21-27: LGTM: Clean assessment template table definition.The table structure is well-defined with appropriate constraints and defaults. Using
timestamp without time zoneforcreated_atandupdated_atis appropriate since these are typically stored in the application's local timezone.
33-38: LGTM: Proper course phase config table with deadline support.The table structure correctly implements:
- UUID primary key for
course_phase_id- Foreign key relationship with cascade delete
- Nullable
deadlinecolumn usingtimestamp with time zonefor proper timezone handling- Clean constraint definition
44-45: LGTM: Appropriate test data.The sample assessment template data provides a good foundation for testing the deadline functionality.
servers/assessment/coursePhaseConfig/router_test.go (8)
21-60: Comprehensive test suite setup.The test suite structure is well-organized with proper database setup, router initialization, and test data preparation. The use of testify suite framework and mock authentication middleware follows Go testing best practices.
68-89: Well-structured positive test case.The test properly sets up test data, makes the HTTP request, and validates both the HTTP status code and the returned deadline value. The use of
time.Equalfor timestamp comparison is appropriate.
91-108: Good coverage of non-existent resource scenario.The test appropriately validates the behavior when requesting a deadline for a non-existent course phase, expecting HTTP 200 with a nil response rather than a 404 error.
110-125: Proper input validation testing.The test correctly validates the handling of invalid UUID format, expecting HTTP 400 status and an appropriate error message. This ensures robust input validation at the API level.
127-152: Comprehensive update operation testing.The test properly validates the PUT endpoint by creating a valid JSON request, checking the HTTP response, and verifying data persistence through the service layer. This ensures end-to-end functionality works correctly.
154-178: Consistent input validation for PUT endpoint.The test appropriately validates input validation for the update endpoint, maintaining consistency with the GET endpoint's error handling behavior.
180-196: Proper JSON validation testing.The test correctly validates the handling of malformed JSON payload, ensuring the API responds appropriately to invalid request formats.
214-216: Standard test suite runner.The test runner follows the standard testify suite pattern correctly.
servers/assessment/coursePhaseConfig/service_test.go (5)
15-47: Consistent test suite setup pattern.The service test setup follows the same well-established pattern as the router tests, maintaining consistency across the test suite. The singleton assignment and test data setup are appropriate.
55-67: Core functionality test is well-implemented.The test properly validates the fundamental update and retrieve operations with appropriate assertions and meaningful error messages.
69-76: Consistent non-existent resource handling.The test properly validates the service-level behavior for non-existent course phases, matching the expectations set by the router tests.
91-115: Excellent test for multiple updates scenario.The test properly validates that multiple updates work correctly and that the latest deadline is always retrieved. Setting up independent test data avoids test dependencies and follows good testing practices.
117-119: Standard test suite runner.The test runner follows the standard testify suite pattern correctly.
|
LGTM |
📝 Introduce deadline for assessment phase
✨ What is the change?
Add a deadline for the assessment phase and update the settings page to include a datepicker to update it.
Also implemented some missing test cases for the action items logic
📌 Reason for the change / Link to issue
closes #575
🧪 How to Test
🖼️ Screenshots (if UI changes are included)
✅ PR Checklist
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Improvements
Backend Enhancements
Bug Fixes