Templating: Copy Course with all Core Data
#525
Conversation
WalkthroughThis change adds a complete "Copy Course" feature, including frontend components, validation, API calls, backend logic, database schema, and tests to duplicate courses with their phases, metadata, and application forms. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Frontend
participant Backend
participant Database
User->>Frontend: Open Edit Course Dropdown
Frontend->>User: Show "Copy Course" option
User->>Frontend: Select "Copy Course"
Frontend->>User: Show CopyCourseDialog form
User->>Frontend: Submit copy form (name, semester, dates)
Frontend->>Backend: POST /courses/:uuid/copy with form data
Backend->>Database: Duplicate course, phases, metadata, forms
Database-->>Backend: Return new course ID
Backend-->>Frontend: Respond with new course info
Frontend->>User: Show success, navigate to new course
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)
✨ Finishing Touches
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 (
|
mathildeshagl
requested a deployment
to
prompt-dev-vm
GitHub Actions
Waiting
…d add course phase handling
mathildeshagl
changed the title
Templating: Copy Course with all Core Data (without Course Phases)Templating: Copy Course with all Core Data
mathildeshagl
temporarily deployed
to
prompt-dev-vm
GitHub Actions
Inactive
… update course creation logic
mathildeshagl
requested a deployment
to
prompt-dev-vm
GitHub Actions
Waiting
… use 'name' instead of 'courseName', change defaultvalues of input field
mathildeshagl
requested a deployment
to
prompt-dev-vm
GitHub Actions
Waiting
mathildeshagl
requested a deployment
to
prompt-dev-vm
GitHub Actions
Waiting
mathildeshagl
requested a deployment
to
prompt-dev-vm
GitHub Actions
Waiting
…and delete functionalities
…context menu interactions
mathildeshagl
temporarily deployed
to
prompt-dev-vm
GitHub Actions
Inactive
mathildeshagl
requested a deployment
to
prompt-dev-vm
GitHub Actions
Waiting
…ng and improve phase ID mapping validation
…data graph copying
mathildeshagl
requested a deployment
to
prompt-dev-vm
GitHub Actions
Waiting
…d UpdateApplicationFormHelper for question management
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
servers/core/course/copy_course.go (4)
297-353: Remove debug logging from production code.The debug log at line 299 could potentially log sensitive application form data. Consider removing this or set it to a more appropriate log level.
- log.Info("Copying application form: ", applicationForm) + // Only log in debug mode if needed + log.Debug("Copying application form")
379-386: Remove debug logging from production code.Similar to the previous comment, the debug log at line 381 should be removed or changed to a more appropriate log level.
- log.Info("Application phase: ", applicationPhaseID) + // Only log in debug mode if needed + log.Debug("Found application phase ID")
412-488: Improve error messages in UpdateApplicationFormHelper.The error messages in this helper function could be more descriptive by including the original error details, which would aid in debugging.
- return errors.New("could not delete question") + return fmt.Errorf("could not delete question with ID %s: %w", questionID, err)Similar improvements could be made for the other error messages in this function.
355-410: Consider refactoring duplicate helper functions.
UpdateParticipationDataGraphHelperandUpdatePhaseDataGraphHelpershare very similar logic. Consider creating a generic helper function that can handle both types of graph updates to reduce duplication.func updateDataGraphHelper( ctx context.Context, qtx *db.Queries, courseID uuid.UUID, graphUpdate []courseDTO.MetaDataGraphItem, deleteFunc func(context.Context, uuid.UUID) error, createFunc func(context.Context, db.CreatePhaseDataConnectionParams) error) error { // Delete all previous connections if err := deleteFunc(ctx, courseID); err != nil { return err } // Create new connections for _, graphItem := range graphUpdate { params := db.CreatePhaseDataConnectionParams{ FromCoursePhaseID: graphItem.FromCoursePhaseID, ToCoursePhaseID: graphItem.ToCoursePhaseID, FromCoursePhaseDtoID: graphItem.FromCoursePhaseDtoID, ToCoursePhaseDtoID: graphItem.ToCoursePhaseDtoID, } if err := createFunc(ctx, params); err != nil { log.Error("Error creating graph connection: ", err) return err } } return nil }Then you could call this function from both current helpers with the appropriate delete and create functions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (3)
servers/core/course/copy_course.go(1 hunks)servers/core/db/query/application.sql(1 hunks)servers/core/db/sqlc/application.sql.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- servers/core/db/query/application.sql
🧰 Additional context used
🧬 Code Graph Analysis (1)
servers/core/db/sqlc/application.sql.go (1)
servers/core/db/sqlc/db.go (1)
Queries(24-26)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- 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 / team-allocation / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-server-team-allocation
- GitHub Check: build-servers / self-team-allocation / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-server-self-team-allocation
- GitHub Check: build-servers / team-allocation / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-server-team-allocation
- GitHub Check: build-servers / self-team-allocation / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-server-self-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 / core / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-server-core
- GitHub Check: build-servers / intro-course / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-server-intro-course
- GitHub Check: build-servers / core / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-server-core
🔇 Additional comments (7)
servers/core/db/sqlc/application.sql.go (1)
606-620: Looks good: New SQL query to retrieve application phase ID.The
GetApplicationPhaseIDForCoursequery and its implementation correctly retrieves the ID of the 'Application' phase for a given course, which is essential for the course copying functionality.servers/core/course/copy_course.go (6)
20-115: The main course copy function looks good, with proper transaction management.The
copyCourseInternalfunction correctly implements a transactional approach to course copying, preserving key course attributes while generating new IDs. The deferred transaction rollback is a good safeguard against partial failures.
117-161: Proper implementation of course phase copying.The
copyCoursePhasesfunction effectively creates new course phases with new IDs while maintaining the properties of the original phases. The mapping from old to new IDs is crucial for the subsequent graph reconstruction.
163-182: Phase graph relationship preservation looks good.The function correctly reconstructs the graph connections between phases using the phase ID mapping.
184-201: Initial phase setting looks correct.The function properly identifies and sets the initial phase for the new course based on the source course configuration.
203-244: DTO mapping implementation looks good.This implementation correctly maps DTOs from the source course by creating an identity map, which is appropriate given that DTOs are global across courses (as confirmed in past review comments).
246-295: Comprehensive meta graph copying logic.The function handles both phase data and participation data graphs, correctly translating IDs using the provided mappings.
mathildeshagl
requested a deployment
to
prompt-dev-vm
GitHub Actions
Waiting
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
servers/core/course/copy_course.go (2)
202-243: Consider checking if the DTOs need to be course-specific.The current implementation of
copyDTOsmaps DTO IDs to themselves (dtoIDMap[o.ID] = o.ID), meaning the cloned course will reference the same DTOs as the original course. If DTOs should be course-specific, you'd need to create new DTOs for the cloned course.Based on the previous discussion in PR comments, the DTOs are confirmed to be global in this architecture, so this implementation is appropriate. The shared DTO approach avoids unnecessary duplication while maintaining proper references in the graph structure.
116-160:⚠️ Potential issueThe GetCoursePhaseByID call breaks transaction isolation.
Within the transaction, you're calling
GetCoursePhaseByIDwhich usesCoursePhaseServiceSingleton.queriesrather than the transaction'sqtx. This could lead to inconsistencies if concurrent modifications occur during copying.Modify the code to either:
- Pass
qtxto a transaction-aware version ofGetCoursePhaseByID, or- Fetch all required phase data using
qtxdirectly within this function.- phase, err := coursePhase.GetCoursePhaseByID(ctx, oldID) + // Option 1: Create a transaction-aware version of GetCoursePhaseByID + dbPhase, err := qtx.GetCoursePhase(ctx, oldID) + if err != nil { + return nil, err + } + phase, err := coursePhaseDTO.GetCoursePhaseDTOFromDBModel(dbPhase)
🧹 Nitpick comments (7)
servers/core/course/copy_course.go (7)
88-89: Fix typo in variable name.The variable name has a typo ("Aplication" instead of "Application").
- sourceAplicationPhaseID, err := getApplicationPhaseID(ctx, qtx, sourceCourseID) + sourceApplicationPhaseID, err := getApplicationPhaseID(ctx, qtx, sourceCourseID)
98-101: Update variable reference to match the renamed variable.This needs to be updated if you apply the variable rename suggestion above.
- err = copyApplicationForm(ctx, qtx, sourceAplicationPhaseID, targetApplicationPhaseID) + err = copyApplicationForm(ctx, qtx, sourceApplicationPhaseID, targetApplicationPhaseID)
296-299: Remove debug logging from production code.The Info-level log statement will produce noise in production logs. Either remove it or change to Debug level.
applicationForm, err := GetApplicationFormHelper(ctx, qtx, sourceCoursePhaseID) - log.Info("Copying application form: ", applicationForm) if err != nil { return err }
378-385: Remove debug logging from production code.The Info-level log statement will produce noise in production logs. Either remove it or change to Debug level.
applicationPhaseID, err := qtx.GetApplicationPhaseIDForCourse(ctx, courseID) - log.Info("Application phase: ", applicationPhaseID) if err != nil { return uuid.Nil, err } return applicationPhaseID, nil
354-376: Consolidate duplicate graph helper functions.
UpdateParticipationDataGraphHelperandUpdatePhaseDataGraphHelperhave nearly identical implementations. Consider consolidating them into a single function that takes the delete and create functions as parameters.+func UpdateDataGraphHelper( + ctx context.Context, + qtx *db.Queries, + courseID uuid.UUID, + graphUpdate []courseDTO.MetaDataGraphItem, + deleteConnections func(context.Context, uuid.UUID) error, + createConnection func(context.Context, interface{}) error, +) error { + // delete all previous connections + err := deleteConnections(ctx, courseID) + if err != nil { + return err + } + + // create new connections + for _, graphItem := range graphUpdate { + params := struct { + FromCoursePhaseID uuid.UUID + ToCoursePhaseID uuid.UUID + FromCoursePhaseDtoID uuid.UUID + ToCoursePhaseDtoID uuid.UUID + }{ + FromCoursePhaseID: graphItem.FromCoursePhaseID, + ToCoursePhaseID: graphItem.ToCoursePhaseID, + FromCoursePhaseDtoID: graphItem.FromCoursePhaseDtoID, + ToCoursePhaseDtoID: graphItem.ToCoursePhaseDtoID, + } + err = createConnection(ctx, params) + if err != nil { + log.Error("Error creating graph connection: ", err) + return err + } + } + return nil +}Then update the calling code:
-func UpdateParticipationDataGraphHelper(ctx context.Context, qtx *db.Queries, courseID uuid.UUID, graphUpdate []courseDTO.MetaDataGraphItem) error { - // delete all previous connections - err := qtx.DeleteParticipationDataGraphConnections(ctx, courseID) - if err != nil { - return err - } - - // create new connections - for _, graphItem := range graphUpdate { - err = qtx.CreateParticipationDataConnection(ctx, db.CreateParticipationDataConnectionParams{ - FromCoursePhaseID: graphItem.FromCoursePhaseID, - ToCoursePhaseID: graphItem.ToCoursePhaseID, - FromCoursePhaseDtoID: graphItem.FromCoursePhaseDtoID, - ToCoursePhaseDtoID: graphItem.ToCoursePhaseDtoID, - }) - if err != nil { - log.Error("Error creating graph connection: ", err) - return err - } - } - return nil -} +func UpdateParticipationDataGraphHelper(ctx context.Context, qtx *db.Queries, courseID uuid.UUID, graphUpdate []courseDTO.MetaDataGraphItem) error { + return UpdateDataGraphHelper( + ctx, qtx, courseID, graphUpdate, + qtx.DeleteParticipationDataGraphConnections, + func(ctx context.Context, p interface{}) error { + params := p.(struct { + FromCoursePhaseID uuid.UUID + ToCoursePhaseID uuid.UUID + FromCoursePhaseDtoID uuid.UUID + ToCoursePhaseDtoID uuid.UUID + }) + return qtx.CreateParticipationDataConnection(ctx, db.CreateParticipationDataConnectionParams(params)) + }, + ) +}Do the same for
UpdatePhaseDataGraphHelper.Also applies to: 387-409
19-114: Consider adding transaction timeouts and improved error handling.The main function is long and contains many database operations that could benefit from having a timeout context. Additionally, you could improve error messages with more context about which operation failed.
Consider:
- Using
db.GetTimeoutContextlike inGetApplicationFormHelper- Adding more context to error returns
func copyCourseInternal(ctx context.Context, sourceCourseID uuid.UUID, courseVariables courseDTO.CopyCourse, requesterID string) (courseDTO.Course, error) { + ctxWithTimeout, cancel := db.GetTimeoutContext(ctx) + defer cancel() sourceCourse, err := CourseServiceSingleton.queries.GetCourse(ctx, sourceCourseID) if err != nil { return courseDTO.Course{}, fmt.Errorf("failed to fetch source course: %w", err) } // ... rest of function ... phaseIDMap, err := copyCoursePhases(ctx, qtx, sourceCourseID, createdCourse.ID) if err != nil { - return courseDTO.Course{}, err + return courseDTO.Course{}, fmt.Errorf("failed to copy course phases: %w", err) } // ... similar improvements for other error handling ... }
411-487: Error messages in UpdateApplicationFormHelper could be more specific.The error messages in this function are generic and don't provide enough context about which specific question caused the error.
Improve error messages to include context about the specific operation and question:
- return errors.New("could not create question") + return fmt.Errorf("could not create text question (title: %s): %w", question.Title, err)Apply similar improvements to other error messages in this function.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (3)
servers/core/course/copy_course.go(1 hunks)servers/core/course/copy_course_test.go(1 hunks)servers/core/database_dumps/copy_course_test.sql(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
servers/core/course/copy_course.go (10)
servers/core/course/service.go (4)
CourseServiceSingleton(26-26)GetCoursePhaseGraph(230-244)GetPhaseDataGraph(264-280)GetParticipationDataGraph(246-262)servers/core/course/courseDTO/get_course.go (1)
GetCourseDTOFromDBModel(23-47)servers/core/coursePhase/service.go (1)
GetCoursePhaseByID(22-29)servers/core/db/sqlc/course_graph.sql.go (2)
CreateCourseGraphConnectionParams(20-23)UpdateInitialCoursePhaseParams(198-201)servers/core/applicationAdministration/applicationDTO/create_question_text.go (1)
CreateQuestionText(9-22)servers/core/applicationAdministration/applicationDTO/create_question_multi_select.go (1)
CreateQuestionMultiSelect(9-23)servers/core/applicationAdministration/applicationDTO/question_text.go (1)
QuestionText(9-23)servers/core/applicationAdministration/applicationDTO/question_multi_select.go (1)
QuestionMultiSelect(9-24)servers/core/db/sqlc/meta_data_graph.sql.go (2)
CreateParticipationDataConnectionParams(19-24)CreatePhaseDataConnectionParams(41-46)servers/core/applicationAdministration/applicationDTO/form.go (2)
Form(5-8)GetFormDTOFromDBModel(10-25)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: lint-servers / lint-server (servers/assessment)
- GitHub Check: lint-servers / lint-server (servers/intro_course)
- GitHub Check: lint-servers / lint-server (servers/self_team_allocation)
- GitHub Check: lint-servers / lint-server (servers/team_allocation)
- GitHub Check: lint-clients / lint-client (devops_challenge_component)
- GitHub Check: lint-clients / lint-client (assessment_component)
- GitHub Check: lint-clients / lint-client (interview_component)
- GitHub Check: lint-clients / lint-client (matching_component)
- GitHub Check: lint-clients / lint-client (intro_course_developer_component)
🔇 Additional comments (2)
servers/core/database_dumps/copy_course_test.sql (1)
1-990: Well-structured database schema for testing course copy functionality.This comprehensive database dump provides all necessary tables, constraints, and test data for verifying the course copying functionality. The schema includes courses, phases, graphs, DTOs, and application questions - all elements that need to be properly copied.
servers/core/course/copy_course_test.go (1)
1-106: The test coverage looks good and verifies key aspects of course copying.The test suite properly verifies that all important components of a course are copied correctly: course attributes, phase graph, initial phase, metadata graphs, and application form questions.
mathildeshagl
requested a deployment
to
prompt-dev-vm
GitHub Actions
Waiting
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
servers/core/course/copy_course.go (1)
224-245: DTO mapping implementation is correct for global DTOs.Based on the previous review discussion, DTOs are global entities, so the identity mapping (
dtoIDMap[o.ID] = o.ID) is the correct approach. This ensures the copied course references the same global DTOs without unnecessary duplication.
🧹 Nitpick comments (6)
clients/core/src/network/mutations/copyCourse.ts (2)
10-10: Fix return type annotation.The return type
Promise<string | undefined>suggests the function can returnundefined, but the implementation on line 19 always returnsdata.id(a string) or throws an error. Theundefinedtype is misleading.-): Promise<string | undefined> => { +): Promise<string> => {
20-23: Enhance error handling with more context.The current error handling only logs the error and re-throws it. Consider adding more context about what operation failed to improve debugging.
} catch (err) { - console.error(err) + console.error('Failed to copy course:', { courseID, error: err }) throw err }servers/core/course/copy_course.go (3)
91-91: Fix typo in variable name.There's a typo in the variable name
sourceAplicationPhaseID- it should besourceApplicationPhaseID.- sourceAplicationPhaseID, err := getApplicationPhaseID(ctx, qtx, sourceCourseID) + sourceApplicationPhaseID, err := getApplicationPhaseID(ctx, qtx, sourceCourseID)And update the usage on line 101:
- err = copyApplicationForm(ctx, qtx, sourceAplicationPhaseID, targetApplicationPhaseID) + err = copyApplicationForm(ctx, qtx, sourceApplicationPhaseID, targetApplicationPhaseID)
34-35: Remove verbose logging statements.The debug logging of start and end dates might be too verbose for production. Consider removing these or using a debug-level logger.
- log.Info("Start date : ", courseVariables.StartDate) - log.Info("End date : ", courseVariables.EndDate)
301-301: Remove verbose logging in production code.The logging statements in
copyApplicationFormandgetApplicationPhaseIDmight be too verbose for production use.- log.Info("Copying application form: ", applicationForm)- log.Info("Application phase: ", applicationPhaseID)Also applies to: 383-383
clients/core/src/managementConsole/courseOverview/interfaces/copyCourse.ts (1)
8-14: Consider exportingformatDatefor reusability.The
formatDatefunction could be useful in other parts of the application. Consider exporting it to promote code reuse.-function formatDate(date: Date): string { +export function formatDate(date: Date): string {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
clients/core/src/managementConsole/courseOverview/components/CopyCourseDialog.tsx(1 hunks)clients/core/src/managementConsole/courseOverview/interfaces/copyCourse.ts(1 hunks)clients/core/src/network/mutations/copyCourse.ts(1 hunks)clients/core/src/validations/copyCourse.ts(1 hunks)servers/core/course/copy_course.go(1 hunks)servers/core/course/copy_course_test.go(1 hunks)servers/core/course/courseDTO/copy_course.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- 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 / self-team-allocation / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-server-self-team-allocation
- 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 / self-team-allocation / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-server-self-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 / 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
- GitHub Check: build-servers / intro-course / Build Docker Image for ghcr.io/ls1intum/prompt2/prompt-server-intro-course
🔇 Additional comments (5)
clients/core/src/network/mutations/copyCourse.ts (1)
16-16:❓ Verification inconclusive
Verify the custom content type header.
The
'application/json-path+json'content type is unusual. Please confirm this is the correct content type expected by the backend API.
🏁 Script executed:
#!/bin/bash # Description: Check if this content type is used elsewhere in the codebase # Expected: Find other usages of this content type or confirm it's backend requirement rg -i "application/json-path\+json" --type ts --type js --type goLength of output: 6460
Confirm Content-Type Usage
The header
'application/json-path+json'appears consistently in 44 other mutation files across the repo (e.g.,clients/core/src/network/mutations/deleteCoursePhase.ts,clients/shared_library/network/mutations/sendStatusMail.ts,clients/team_allocation_component/src/team_allocation/network/mutations/updateSkill.ts).
• If this is meant to be the standard JSON Patch media type (application/json-patch+json), it needs correcting here and in every occurrence.
• Otherwise, please validate with the backend/API specification that'application/json-path+json'is indeed the intended Content-Type.servers/core/course/courseDTO/copy_course.go (1)
5-10: LGTM! Clean DTO implementation.The
CopyCoursestruct is well-defined with appropriate field types and JSON tags. Usingpgtypefor optional fields likeSemesterTagand dates is the correct approach for PostgreSQL integration.clients/core/src/validations/copyCourse.ts (1)
7-7:❓ Verification inconclusive
Verify business rule for disallowing "-" character.
Both
nameandsemesterTagfields disallow the "-" character. Please confirm this is a valid business requirement, as hyphens are commonly used in course names and semester tags (e.g., "Spring-2024").Also applies to: 15-15
🏁 Script executed:
#!/bin/bash # Description: Check existing course data to see if hyphens are used in names/tags # Expected: Find examples of courses with hyphens to validate this constraint rg -i "name.*-|semester.*-" --type sql --type go -A 2 -B 2Length of output: 67026
Confirm prohibition of “-” in course names and semester tags
Existing code, SQL queries, tests, and even Keycloak role names all assume hyphens are allowed:
- clients/core/src/validations/copyCourse.ts (lines 7 & 15) now rejects any “-”
- Permission string SQL (
permission_strings.sql) and generated Go code useCONCAT(semester_tag, '-', name)- course_test.sql includes names with hyphens (e.g. “iPraktikum-Test”)
- Keycloak realm manager builds group/role names via
fmt.Sprintf("%s-%s", …)Please confirm that disallowing hyphens is an intentional business rule. If so, plan for migrating or sanitizing existing course names/semester tags (and any downstream data or roles) to avoid breaking current entries.
servers/core/course/copy_course.go (1)
48-117: Excellent transaction management and error handling.The function properly uses database transactions with appropriate rollback handling via
utils.DeferRollback(). The sequential copying of course components (phases, graphs, DTOs, metadata, application forms) ensures data consistency, and each step has proper error handling that will trigger a rollback if needed.servers/core/course/copy_course_test.go (1)
1-111: Well-structured and comprehensive test coverage!The test suite thoroughly validates all aspects of the course copying functionality including phases, graphs, and application forms. Good use of test fixtures and proper cleanup.
clients/core/src/managementConsole/courseOverview/interfaces/copyCourse.ts
Show resolved
Hide resolved
clients/core/src/managementConsole/courseOverview/components/CopyCourseDialog.tsx
Show resolved
Hide resolved
clients/core/src/managementConsole/courseOverview/components/CopyCourseDialog.tsx
Show resolved
Hide resolved
clients/core/src/managementConsole/courseOverview/components/CopyCourseDialog.tsx
Show resolved
Hide resolved
mathildeshagl
requested a deployment
to
prompt-dev-vm
GitHub Actions
Waiting
mathildeshagl
temporarily deployed
to
prompt-dev-vm
GitHub Actions
Inactive
clients/core/src/managementConsole/courseOverview/components/CopyCourseDialog.tsx
Show resolved
Hide resolved
| @@ -29,6 +29,8 @@ func setupCourseRouter(router *gin.RouterGroup, authMiddleware func() gin.Handle | |||
| course.GET("/self", getOwnCourses) | |||
|
|
|||
| course.DELETE("/:uuid", permissionIDMiddleware(permissionValidation.PromptAdmin, permissionValidation.CourseLecturer), deleteCourse) | |||
|
|
|||
| course.POST("/:uuid/copy", permissionIDMiddleware(permissionValidation.PromptAdmin, permissionValidation.CourseLecturer), copyCourse) | |||
There was a problem hiding this comment.
I think you're missing a router test for this
📝 Copy Course
This PR adds support for duplicating existing courses both on the server side and in the user interface.
✨ What is the change?
🛠 Core Server
💻 UI Improvements
📌 Reason for the change / Link to issue
#517
🧪 How to Test
🖼️ Screenshots (if UI changes are included)
✅ PR Checklist
Summary by CodeRabbit
Summary by CodeRabbit
/courses/:uuid/copyto initiate course copying.