feat(apps/frontend-manage): course duplication#4954
Draft
jabbadizzleCode wants to merge 2 commits intov3from
Draft
feat(apps/frontend-manage): course duplication#4954jabbadizzleCode wants to merge 2 commits intov3from
jabbadizzleCode wants to merge 2 commits intov3from
Conversation
dosubot
bot
added
the
size:XXL
📝 WalkthroughWalkthroughAdds end-to-end course duplication: frontend modal and header wiring to call createCourse with duplication flags, GraphQL schema extended to route duplication to a new duplicateCourse service, backend service logic to clone courses and optional related activities, and new i18n strings. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Header as CourseOverviewHeader
participant Modal as CourseDuplicationModal
participant Apollo as Apollo Client
participant GraphQL as GraphQL:createCourse
participant Service as CourseService
participant DB as Data Store
User->>Header: Click "Duplicate course"
Header->>Modal: Open modal
User->>Modal: Fill form & submit
Modal->>Header: onSubmit(formData)
Header->>Apollo: mutate(createCourse {id?, duplicateFlags, metadata})
Apollo->>GraphQL: createCourse(args)
alt args.id present
GraphQL->>Service: duplicateCourse(args, ctx)
Service->>DB: Read source course + activities
Service->>DB: Create new course (apply date delta)
opt duplicate flags
Service->>DB: Clone activities (adjusted dates)
end
Service-->>GraphQL: new course
else
GraphQL->>Service: createCourse(args, ctx)
Service->>DB: Create new course
Service-->>GraphQL: new course
end
GraphQL-->>Apollo: course
Apollo-->>Header: mutation result
Header->>Apollo: update GetUserCourses cache (append)
Header-->>Modal: close(success)
Modal-->>User: done
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
packages/graphql/src/graphql/ops/MCreateCourse.graphqlis excluded by!**/**/graphql/ops/**packages/graphql/src/ops.schema.jsonis excluded by!**/**/ops.schema.jsonpackages/graphql/src/ops.tsis excluded by!**/**/ops.tspackages/graphql/src/public/client.jsonis excluded by!**/**/public/**packages/graphql/src/public/schema.graphqlis excluded by!**/**/public/**packages/graphql/src/public/server.jsonis excluded by!**/**/public/**
📒 Files selected for processing (6)
apps/frontend-manage/src/components/courses/CourseOverviewHeader.tsx(6 hunks)apps/frontend-manage/src/components/courses/modals/CourseDuplicationModal.tsx(1 hunks)packages/graphql/src/schema/mutation.ts(1 hunks)packages/graphql/src/services/courses.ts(3 hunks)packages/i18n/messages/de.ts(2 hunks)packages/i18n/messages/en.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
apps/frontend-manage/src/components/courses/modals/CourseDuplicationModal.tsx (2)
packages/graphql/src/ops.ts (2)
Course(883-929)UserProfileDocument(7332-7332)packages/graphql/src/schema/user.ts (1)
LocaleType(4-6)
packages/graphql/src/services/courses.ts (5)
packages/graphql/src/lib/context.ts (1)
ContextWithUser(38-47)packages/graphql/src/services/liveQuizzes.ts (1)
manipulateLiveQuiz(190-569)packages/graphql/src/services/practiceQuizzes.ts (1)
manipulatePracticeQuiz(170-430)packages/graphql/src/services/microLearning.ts (1)
manipulateMicroLearning(185-462)packages/graphql/src/services/groups.ts (1)
manipulateGroupActivity(838-1129)
apps/frontend-manage/src/components/courses/CourseOverviewHeader.tsx (2)
packages/graphql/src/ops.ts (2)
CreateCourseDocument(7099-7099)GetUserCoursesDocument(7320-7320)apps/frontend-manage/src/components/courses/modals/CourseDuplicationModal.tsx (1)
CourseDuplicationFormData(44-62)
🪛 Biome (2.1.2)
apps/frontend-manage/src/components/courses/modals/CourseDuplicationModal.tsx
[error] 221-221: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 244-244: Do not add then to an object.
(lint/suspicious/noThenProperty)
🔇 Additional comments (1)
packages/i18n/messages/en.ts (1)
2724-2724: LGTM!The translation key follows existing naming conventions and provides clear, concise text for the duplicate course action.
Comment on lines
+219
to
+248
| .when('isGroupCreationEnabled', { | ||
| is: true, | ||
| then: (schema) => | ||
| schema.max( | ||
| yup.ref('endDate'), | ||
| t('manage.courseList.groupDeadlineBeforeEnd') | ||
| ), | ||
| otherwise: (schema) => schema, | ||
| }) | ||
| .test( | ||
| 'isBeforeFirstGroupActivity', | ||
| t('manage.courseList.groupDeadlineBeforeFirstGroupActivity', { | ||
| date: dayjs(earliestGroupDeadline).format('DD.MM.YYYY, HH:mm'), | ||
| }), | ||
| (date) => { | ||
| return earliestGroupDeadline | ||
| ? dayjs(date).isBefore(dayjs(earliestGroupDeadline)) | ||
| : true | ||
| } | ||
| ) | ||
| : yup | ||
| .date() | ||
| .min(new Date(), t('manage.courseList.groupDeadlineFuture')) | ||
| .when('isGroupCreationEnabled', { | ||
| is: true, | ||
| then: (schema) => | ||
| schema.max( | ||
| yup.ref('endDate'), | ||
| t('manage.courseList.groupDeadlineBeforeEnd') | ||
| ), |
There was a problem hiding this comment.
Biome lint failure: then property is forbidden
Biome’s lint/suspicious/noThenProperty rule errors on the then keys in the yup.when options object, so the build will fail as-is. Please suppress the rule (e.g., // biome-ignore lint/suspicious/noThenProperty: yup requires the "then" option) or restructure the conditional to satisfy the rule.
🧰 Tools
🪛 Biome (2.1.2)
[error] 221-221: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 244-244: Do not add then to an object.
(lint/suspicious/noThenProperty)
🤖 Prompt for AI Agents
In apps/frontend-manage/src/components/courses/modals/CourseDuplicationModal.tsx
around lines 219 to 248, Biome flags the use of the reserved "then" key inside
the yup.when options object; either add a suppression comment immediately above
each offending then property (for example: // biome-ignore
lint/suspicious/noThenProperty: yup requires the "then" option) or refactor the
conditional to avoid passing an object with a then property (e.g., call when
with a predicate and apply schema transformation imperatively or build the
schema in two steps), ensuring the same validation behavior is preserved.
Comment on lines
+2625
to
+2639
| changeAvailabilityDateMicrolearnings: | ||
| 'Die Verfügbarkeit der Microlearnings werden basierend auf den ursprünglichen Kursstartdatum an die neuen Kursdaten angepasst.', | ||
| changeAvailabilityDateGroupActivities: | ||
| 'Die Verfügbarkeitsdaten der Gruppenaktivitäten werden entsprechend der Verschiebung zum ursprünglichen Kursstartdatum an die neuen Kursdaten angepasst.', | ||
| courseDatesForCourseDuplicationTooltip: | ||
| 'Aus technischen Gründen sind die Kursdaten auf einen fixen Intervall festgelegt, der durch den ursprünglichen Kurs definiert ist. Sie können die Daten für den duplizierten Kurs anschließend ändern.', | ||
| groupCreationDeadlineForCourseDuplicationTooltip: | ||
| 'Aus technischen Gründen ist die Deadline für die Gruppenbildung basierend auf das Kursstartdatum festgelegt. Sie können diese Deadline in den Kurseinstellungen anpassen.', | ||
| copyLiveQuizzesTooltip: | ||
| 'Wenn Sie diese Einstellung aktivieren, werden alle Live-Quizzes im Kurs in den neuen Kurs kopiert.', | ||
| copyPracticeQuizzesTooltip: | ||
| 'Wenn Sie diese Einstellung aktivieren, werden alle Übungs-Quizzes im Kurs in den neuen Kurs kopiert.', | ||
| copyMicroLearningsTooltip: | ||
| 'Wenn Sie diese Einstellung aktivieren, werden alle Micro-Learnings im Kurs in den neuen Kurs kopiert.', | ||
| copyGroupActivitiesTooltip: |
There was a problem hiding this comment.
Fix grammatical errors and terminology consistency in new tooltips.
Bitte korrigieren diese sichtbaren Strings:
- Line 2626: „Die Verfügbarkeit … werden“ → „wird“; „auf den ursprünglichen“ → „auf dem ursprünglichen“.
- Line 2630: „auf einen fixen Intervall“ → „auf ein fixes Intervall“.
- Line 2632: „basierend auf das Kursstartdatum“ → „basierend auf dem Kursstartdatum“.
- Line 2638: Vereinheitliche auf „Microlearnings“ (ohne Bindestrich) wie im restlichen Locale.
- changeAvailabilityDateMicrolearnings:
- 'Die Verfügbarkeit der Microlearnings werden basierend auf den ursprünglichen Kursstartdatum an die neuen Kursdaten angepasst.',
+ changeAvailabilityDateMicrolearnings:
+ 'Die Verfügbarkeit der Microlearnings wird basierend auf dem ursprünglichen Kursstartdatum an die neuen Kursdaten angepasst.',
@@
- courseDatesForCourseDuplicationTooltip:
- 'Aus technischen Gründen sind die Kursdaten auf einen fixen Intervall festgelegt, der durch den ursprünglichen Kurs definiert ist. Sie können die Daten für den duplizierten Kurs anschließend ändern.',
+ courseDatesForCourseDuplicationTooltip:
+ 'Aus technischen Gründen sind die Kursdaten auf ein fixes Intervall festgelegt, das durch den ursprünglichen Kurs definiert ist. Sie können die Daten für den duplizierten Kurs anschließend ändern.',
@@
- groupCreationDeadlineForCourseDuplicationTooltip:
- 'Aus technischen Gründen ist die Deadline für die Gruppenbildung basierend auf das Kursstartdatum festgelegt. Sie können diese Deadline in den Kurseinstellungen anpassen.',
+ groupCreationDeadlineForCourseDuplicationTooltip:
+ 'Aus technischen Gründen ist die Deadline für die Gruppenbildung basierend auf dem Kursstartdatum festgelegt. Sie können diese Deadline in den Kurseinstellungen anpassen.',
@@
- copyMicroLearningsTooltip:
- 'Wenn Sie diese Einstellung aktivieren, werden alle Micro-Learnings im Kurs in den neuen Kurs kopiert.',
+ copyMicroLearningsTooltip:
+ 'Wenn Sie diese Einstellung aktivieren, werden alle Microlearnings im Kurs in den neuen Kurs kopiert.',📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| changeAvailabilityDateMicrolearnings: | |
| 'Die Verfügbarkeit der Microlearnings werden basierend auf den ursprünglichen Kursstartdatum an die neuen Kursdaten angepasst.', | |
| changeAvailabilityDateGroupActivities: | |
| 'Die Verfügbarkeitsdaten der Gruppenaktivitäten werden entsprechend der Verschiebung zum ursprünglichen Kursstartdatum an die neuen Kursdaten angepasst.', | |
| courseDatesForCourseDuplicationTooltip: | |
| 'Aus technischen Gründen sind die Kursdaten auf einen fixen Intervall festgelegt, der durch den ursprünglichen Kurs definiert ist. Sie können die Daten für den duplizierten Kurs anschließend ändern.', | |
| groupCreationDeadlineForCourseDuplicationTooltip: | |
| 'Aus technischen Gründen ist die Deadline für die Gruppenbildung basierend auf das Kursstartdatum festgelegt. Sie können diese Deadline in den Kurseinstellungen anpassen.', | |
| copyLiveQuizzesTooltip: | |
| 'Wenn Sie diese Einstellung aktivieren, werden alle Live-Quizzes im Kurs in den neuen Kurs kopiert.', | |
| copyPracticeQuizzesTooltip: | |
| 'Wenn Sie diese Einstellung aktivieren, werden alle Übungs-Quizzes im Kurs in den neuen Kurs kopiert.', | |
| copyMicroLearningsTooltip: | |
| 'Wenn Sie diese Einstellung aktivieren, werden alle Micro-Learnings im Kurs in den neuen Kurs kopiert.', | |
| copyGroupActivitiesTooltip: | |
| changeAvailabilityDateMicrolearnings: | |
| 'Die Verfügbarkeit der Microlearnings wird basierend auf dem ursprünglichen Kursstartdatum an die neuen Kursdaten angepasst.', | |
| changeAvailabilityDateGroupActivities: | |
| 'Die Verfügbarkeitsdaten der Gruppenaktivitäten werden entsprechend der Verschiebung zum ursprünglichen Kursstartdatum an die neuen Kursdaten angepasst.', | |
| courseDatesForCourseDuplicationTooltip: | |
| 'Aus technischen Gründen sind die Kursdaten auf ein fixes Intervall festgelegt, das durch den ursprünglichen Kurs definiert ist. Sie können die Daten für den duplizierten Kurs anschließend ändern.', | |
| groupCreationDeadlineForCourseDuplicationTooltip: | |
| 'Aus technischen Gründen ist die Deadline für die Gruppenbildung basierend auf dem Kursstartdatum festgelegt. Sie können diese Deadline in den Kurseinstellungen anpassen.', | |
| copyLiveQuizzesTooltip: | |
| 'Wenn Sie diese Einstellung aktivieren, werden alle Live-Quizzes im Kurs in den neuen Kurs kopiert.', | |
| copyPracticeQuizzesTooltip: | |
| 'Wenn Sie diese Einstellung aktivieren, werden alle Übungs-Quizzes im Kurs in den neuen Kurs kopiert.', | |
| copyMicroLearningsTooltip: | |
| 'Wenn Sie diese Einstellung aktivieren, werden alle Microlearnings im Kurs in den neuen Kurs kopiert.', | |
| copyGroupActivitiesTooltip: |
🤖 Prompt for AI Agents
In packages/i18n/messages/de.ts around lines 2625–2639, fix grammar and
terminology in the tooltip strings: change line 2626 "Die Verfügbarkeit der
Microlearnings werden" → use singular verb "wird" and "auf den ursprünglichen" →
"auf dem ursprünglichen"; change line 2630 "auf einen fixen Intervall" → "auf
ein fixes Intervall"; change line 2632 "basierend auf das Kursstartdatum" →
"basierend auf dem Kursstartdatum"; and normalize spelling on line 2638 to
"Microlearnings" (no hyphen) to match the rest of the locale.
Comment on lines
+2586
to
+2589
| changeAvailabilityDateMicrolearnings: | ||
| 'The availability of micro learnings will be adjusted according to the new course dates based on the offset to the original course start date.', | ||
| changeAvailabilityDateGroupActivities: | ||
| 'The availability of group activities will be adjusted according to the new course dates based on the offset to the original course start date.', |
There was a problem hiding this comment.
Fix terminology inconsistency: "micro learnings" → "microlearnings".
Line 2587 uses "micro learnings" (two words), but the rest of the codebase consistently uses "microlearnings" as a single compound word (see lines 201, 202, 429, 2599, etc.).
Apply this diff:
- changeAvailabilityDateMicrolearnings:
- 'The availability of micro learnings will be adjusted according to the new course dates based on the offset to the original course start date.',
+ changeAvailabilityDateMicrolearnings:
+ 'The availability of microlearnings will be adjusted according to the new course dates based on the offset to the original course start date.',📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| changeAvailabilityDateMicrolearnings: | |
| 'The availability of micro learnings will be adjusted according to the new course dates based on the offset to the original course start date.', | |
| changeAvailabilityDateGroupActivities: | |
| 'The availability of group activities will be adjusted according to the new course dates based on the offset to the original course start date.', | |
| changeAvailabilityDateMicrolearnings: | |
| 'The availability of microlearnings will be adjusted according to the new course dates based on the offset to the original course start date.', | |
| changeAvailabilityDateGroupActivities: | |
| 'The availability of group activities will be adjusted according to the new course dates based on the offset to the original course start date.', |
🤖 Prompt for AI Agents
In packages/i18n/messages/en.ts around lines 2586 to 2589, the string for
changeAvailabilityDateMicrolearnings uses the incorrect term "micro learnings"
(two words); update it to the consistent project terminology "microlearnings"
(one word) so it matches other occurrences (e.g., lines 201, 202, 429, 2599),
leaving the rest of the sentence unchanged.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
packages/i18n/messages/de.ts (1)
2630-2643: Bitte die bereits erwähnten Grammatikfehler korrigieren.Die Strings enthalten weiterhin die gemeldeten Fehler („Die Verfügbarkeit … wird“, „… basierend auf dem Kursstartdatum“, „… auf ein fixes Intervall …“, „Microlearnings“ ohne Bindestrich). Bitte die vorgeschlagenen Formulierungen aus der vorherigen Rückmeldung übernehmen.
packages/graphql/src/services/courses.ts (1)
2735-2778: Critical: Missing permission check enables unauthorized course access.This is the same critical security vulnerability flagged in the previous review (see past review comments). The course and all its activities are loaded before verifying that
ctx.user.subhas permission to access it. Any authenticated user can call this function with an arbitrary course ID and receive a full copy of someone else's course content.Add a permission check before fetching the course:
+ // Verify user has access to the source course + const hasAccess = await ctx.prisma.derivedPermission.findFirst({ + where: { + courseId: id, + userId: ctx.user.sub, + permissionLevel: { + in: [DB.PermissionLevel.OWNER, DB.PermissionLevel.ADMIN, DB.PermissionLevel.WRITE], + }, + }, + }) + + if (!hasAccess) { + throw new GraphQLError('Insufficient permissions to duplicate this course') + } + const oldCourse = await ctx.prisma.course.findUnique({ where: { id },Alternatively, include the permission check in the
whereclause:const oldCourse = await ctx.prisma.course.findUnique({ - where: { id }, + where: { + id, + permissions: { + some: { + userId: ctx.user.sub, + permissionLevel: { + in: [DB.PermissionLevel.OWNER, DB.PermissionLevel.ADMIN, DB.PermissionLevel.WRITE], + }, + }, + }, + },packages/i18n/messages/en.ts (2)
2591-2592: Fix terminology inconsistency: "micro learnings" → "microlearnings".Line 2591 uses "micro learnings" (two words), but the codebase consistently uses "microlearnings" as a single compound word (see lines 201, 202, 429, etc.). This inconsistency was previously flagged in past review comments but appears in this new code.
Apply this diff:
changeAvailabilityDateMicrolearnings: - 'The availability of micro learnings will be adjusted according to the new course dates based on the offset to the original course start date.', + 'The availability of microlearnings will be adjusted according to the new course dates based on the offset to the original course start date.',
2603-2604: Fix terminology inconsistency: "micro learnings" → "microlearnings".Line 2603 uses "micro learnings" (two words), but the codebase consistently uses "microlearnings" as a single compound word (see lines 201, 202, 429, etc.).
Apply this diff:
copyMicroLearningsTooltip: - 'If you activate this setting, all micro learnings in the course will be copied to the new course.', + 'If you activate this setting, all microlearnings in the course will be copied to the new course.',
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
packages/graphql/src/ops.schema.jsonis excluded by!**/**/ops.schema.jsonpackages/graphql/src/ops.tsis excluded by!**/**/ops.tspackages/graphql/src/public/client.jsonis excluded by!**/**/public/**packages/graphql/src/public/schema.graphqlis excluded by!**/**/public/**packages/graphql/src/public/server.jsonis excluded by!**/**/public/**
📒 Files selected for processing (4)
apps/frontend-manage/src/components/courses/CourseOverviewHeader.tsx(6 hunks)packages/graphql/src/services/courses.ts(3 hunks)packages/i18n/messages/de.ts(2 hunks)packages/i18n/messages/en.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
apps/frontend-manage/src/components/courses/CourseOverviewHeader.tsx (2)
packages/graphql/src/ops.ts (2)
CreateCourseDocument(7141-7141)GetUserCoursesDocument(7364-7364)apps/frontend-manage/src/components/courses/modals/CourseDuplicationModal.tsx (1)
CourseDuplicationFormData(44-62)
packages/graphql/src/services/courses.ts (5)
packages/graphql/src/lib/context.ts (1)
ContextWithUser(38-47)packages/graphql/src/services/liveQuizzes.ts (1)
manipulateLiveQuiz(190-569)packages/graphql/src/services/practiceQuizzes.ts (1)
manipulatePracticeQuiz(170-430)packages/graphql/src/services/microLearning.ts (1)
manipulateMicroLearning(185-462)packages/graphql/src/services/groups.ts (1)
manipulateGroupActivity(838-1129)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: cypress-run-cloud
🔇 Additional comments (3)
packages/graphql/src/services/courses.ts (2)
27-30: LGTM!The imports for activity manipulation functions are correctly added to support the course duplication feature.
2616-2621: LGTM!The interface extension correctly adds the necessary fields to support course duplication with granular control over which activity types to duplicate.
packages/i18n/messages/en.ts (1)
2593-2606: LGTM for the remaining tooltip strings.The other tooltip strings (lines 2593-2606) are grammatically correct, use consistent terminology for "group activities", "live quizzes", and "practice quizzes", and provide clear explanations for the course duplication workflow.
Comment on lines
+264
to
+273
| maxGroupSize: parseInt(String(values.maxGroupSize)), | ||
| preferredGroupSize: parseInt( | ||
| String(values.preferredGroupSize) | ||
| ), | ||
| id: course.id, | ||
| duplicateLiveQuizzes: values.copyLiveQuizzes, | ||
| duplicatePracticeQuizzes: values.copyPracticeQuizzes, | ||
| duplicateMicrolearnings: values.copyMicroLearnings, | ||
| duplicateGroupActivities: values.copyGroupActivities, | ||
| }, |
There was a problem hiding this comment.
Guard optional group size fields before calling the mutation.
parseInt(String(values.maxGroupSize)) (und analog für preferredGroupSize) ergibt bei undefined/null einen NaN, der als null serialisiert wird. Da maxGroupSize/preferredGroupSize im Mutation-Input non-null sind, schlägt die Duplizierung z.B. bei Kursen ohne Gruppenbildung sofort mit einem GraphQL-Fehler fehl. Bitte nur numerische Werte senden und ansonsten auf den bestehenden Kurswert zurückfallen oder das Feld ganz weglassen.
- maxGroupSize: parseInt(String(values.maxGroupSize)),
- preferredGroupSize: parseInt(
- String(values.preferredGroupSize)
- ),
+ ...(values.maxGroupSize ?? course.maxGroupSize
+ ? {
+ maxGroupSize: Number(
+ values.maxGroupSize ?? course.maxGroupSize
+ ),
+ }
+ : {}),
+ ...(values.preferredGroupSize ?? course.preferredGroupSize
+ ? {
+ preferredGroupSize: Number(
+ values.preferredGroupSize ?? course.preferredGroupSize
+ ),
+ }
+ : {}),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| maxGroupSize: parseInt(String(values.maxGroupSize)), | |
| preferredGroupSize: parseInt( | |
| String(values.preferredGroupSize) | |
| ), | |
| id: course.id, | |
| duplicateLiveQuizzes: values.copyLiveQuizzes, | |
| duplicatePracticeQuizzes: values.copyPracticeQuizzes, | |
| duplicateMicrolearnings: values.copyMicroLearnings, | |
| duplicateGroupActivities: values.copyGroupActivities, | |
| }, | |
| ...(values.maxGroupSize ?? course.maxGroupSize | |
| ? { | |
| maxGroupSize: Number( | |
| values.maxGroupSize ?? course.maxGroupSize | |
| ), | |
| } | |
| : {}), | |
| ...(values.preferredGroupSize ?? course.preferredGroupSize | |
| ? { | |
| preferredGroupSize: Number( | |
| values.preferredGroupSize ?? course.preferredGroupSize | |
| ), | |
| } | |
| : {}), | |
| id: course.id, | |
| duplicateLiveQuizzes: values.copyLiveQuizzes, | |
| duplicatePracticeQuizzes: values.copyPracticeQuizzes, | |
| duplicateMicrolearnings: values.copyMicroLearnings, | |
| duplicateGroupActivities: values.copyGroupActivities, | |
| }, |
🤖 Prompt for AI Agents
In apps/frontend-manage/src/components/courses/CourseOverviewHeader.tsx around
lines 264 to 273, the code unconditionally calls
parseInt(String(values.maxGroupSize)) and
parseInt(String(values.preferredGroupSize)) which produce NaN for undefined/null
and end up serialized as null, causing GraphQL errors because the mutation input
requires non-null; change this to only send numeric values: if
values.maxGroupSize (or values.preferredGroupSize) is a finite number/string
that parses to a valid integer, include the parsed integer, otherwise fall back
to the existing course.maxGroupSize (course.preferredGroupSize) or omit the
field from the mutation payload so that we never send NaN/null for these
non-nullable inputs.
Comment on lines
+2914
to
+2920
| const stack = oldGroupActivity.stacks[0] | ||
| if (!stack) { | ||
| console.log( | ||
| `Failed fetch stack for group activity ${oldGroupActivity.id}` | ||
| ) | ||
| return newCourse | ||
| } |
There was a problem hiding this comment.
Improve error handling for missing group activity stacks.
When a group activity stack is missing, the function logs to console and returns early, silently skipping remaining group activities. This can result in incomplete duplication without clear feedback to the user.
Consider throwing an error or using proper logging and continuing:
const stack = oldGroupActivity.stacks[0]
if (!stack) {
- console.log(
- `Failed fetch stack for group activity ${oldGroupActivity.id}`
- )
- return newCourse
+ // Log with proper logger and continue with next activity
+ console.error(
+ `[duplicateCourse] Missing stack for group activity ${oldGroupActivity.id}, skipping`
+ )
+ continue
}Or throw an error if stack is required:
const stack = oldGroupActivity.stacks[0]
if (!stack) {
- console.log(
- `Failed fetch stack for group activity ${oldGroupActivity.id}`
- )
- return newCourse
+ throw new GraphQLError(
+ `Group activity ${oldGroupActivity.id} is missing required stack data`
+ )
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const stack = oldGroupActivity.stacks[0] | |
| if (!stack) { | |
| console.log( | |
| `Failed fetch stack for group activity ${oldGroupActivity.id}` | |
| ) | |
| return newCourse | |
| } | |
| const stack = oldGroupActivity.stacks[0] | |
| if (!stack) { | |
| // Log with proper logger and continue with next activity | |
| console.error( | |
| `[duplicateCourse] Missing stack for group activity ${oldGroupActivity.id}, skipping` | |
| ) | |
| continue | |
| } |
Suggested change
| const stack = oldGroupActivity.stacks[0] | |
| if (!stack) { | |
| console.log( | |
| `Failed fetch stack for group activity ${oldGroupActivity.id}` | |
| ) | |
| return newCourse | |
| } | |
| const stack = oldGroupActivity.stacks[0] | |
| if (!stack) { | |
| throw new GraphQLError( | |
| `Group activity ${oldGroupActivity.id} is missing required stack data` | |
| ) | |
| } |
🤖 Prompt for AI Agents
In packages/graphql/src/services/courses.ts around lines 2914 to 2920, the code
currently console.logs and returns early when oldGroupActivity.stacks[0] is
missing, which silently aborts duplication; replace this with explicit error
handling: either throw a descriptive Error (including oldGroupActivity.id and
course/context identifiers) if a stack is required so the caller can surface the
failure, or log a structured error via the project logger and continue to the
next group activity (not return newCourse) to avoid aborting the whole
process—choose one behavior, implement it consistently, and ensure any thrown
error is typed or documented for upstream handling.
klicker-uzh
|
||||||||||||||||||||||||||||
| Project |
klicker-uzh
|
| Branch Review |
course-duplication
|
| Run status |
|
| Run duration | 10m 21s |
| Commit |
|
| Committer | Patrick Louis |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
1
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
757
|
| View all changes introduced in this branch ↗︎ | |
Tests for review
cypress/e2e/N-course-workflow.cy.ts • 1 failed test
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Chores