feat: add possibility to archive past courses #4281
Conversation
Current Aviator status
This PR was merged manually (without Aviator). Merging manually can negatively impact the performance of the queue. Consider using Aviator next time.
See the real-time status of this PR on the
Aviator webapp.
Use the Aviator Chrome Extension
to see the status of your PR within GitHub.
|
📝 WalkthroughWalkthroughThis pull request introduces several modifications across various components and services in the frontend and GraphQL schema. Key updates include the addition of new props for the Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
klicker-uzh
|
||||||||||||||||||||||||||||
| Project |
klicker-uzh
|
| Branch Review |
course-archive
|
| Run status |
|
| Run duration | 08m 59s |
| Commit |
|
| Committer | Julius Schlapbach |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
45
|
| View all changes introduced in this branch ↗︎ | |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (30)
packages/graphql/src/graphql/ops/QGetUserCourses.graphql (1)
6-13: LGTM! Consider optimizing the query structure.The additions of
color,startDate, andendDatefields to theGetUserCoursesquery are well-placed and align with the PR objective of adding the possibility to archive past courses. These new fields will provide valuable information for course management and UI customization.Consider grouping related fields together for better readability. For example:
query GetUserCourses { userCourses { id name displayName color pinCode description isArchived isGamificationEnabled isGroupCreationEnabled startDate endDate createdAt updatedAt } }This structure groups identifier fields, descriptive fields, boolean flags, and date fields together.
packages/graphql/src/graphql/ops/QGetActiveUserCourses.graphql (1)
1-17: LGTM! The query structure and field selection are well-defined.The
GetActiveUserCoursesquery is well-structured and includes a comprehensive set of fields for active user courses. This aligns well with the PR objective of adding the possibility to archive past courses, as evidenced by the inclusion of theisArchivedfield.For consistency, consider renaming the query to
getActiveUserCourses(lowercase 'get') to match the resolver name. This follows the common GraphQL naming convention where query names typically start with lowercase letters.-query GetActiveUserCourses { +query getActiveUserCourses { getActiveUserCourses { # ... fields remain the same } }apps/frontend-manage/src/components/courses/modals/CourseArchiveModal.tsx (2)
16-26: LGTM: Component setup and hooks usage are correct.The component function and hooks are well-implemented. The useMutation hook correctly includes the refetchQueries option for data consistency.
Consider destructuring the
tfunction fromuseTranslationsto make it clear which translation namespace is being used:- const t = useTranslations() + const { t } = useTranslations('yourNamespace')This change would improve code clarity and make it easier to manage translations across different namespaces if needed in the future.
32-86: LGTM: Modal implementation is well-structured with good UX considerations.The Modal component is correctly implemented with dynamic content based on the
isArchivedstatus. The use of optimistic response in the mutation is a great practice for improving perceived performance. The inclusion of data-cy attributes facilitates easier testing.Consider adding error handling to the mutation to improve user experience in case of network issues or server errors:
onClick={async () => { await toggleArchiveCourse({ variables: { id: courseId, isArchived: !isArchived }, optimisticResponse: { __typename: 'Mutation', toggleArchiveCourse: { __typename: 'Course', id: courseId, isArchived: !isArchived, }, }, + }).catch((error) => { + console.error('Error toggling course archive status:', error); + // TODO: Add user-friendly error notification here }) setOpen(false) }}This addition will help catch and log any errors that occur during the mutation, allowing for better debugging and potentially informing the user about the failure.
apps/frontend-manage/src/components/sessions/creation/ElementCreation.tsx (1)
Line range hint
112-124: LGTM! Consider a minor readability improvement.The data mapping has been correctly updated to use
getActiveUserCoursesinstead ofuserCourses, which is consistent with the new query structure. The mapping logic and type annotations are correct.For improved readability, consider extracting the
Picktype into a separate type alias. This would make the code more maintainable and easier to read. Here's a suggested change:+ type CourseData = Pick< + Course, + 'id' | 'name' | 'isGamificationEnabled' | 'isGroupCreationEnabled' + > const courseSelection = useMemo( () => dataCourses?.getActiveUserCourses?.map( - ( - course: Pick< - Course, - 'id' | 'name' | 'isGamificationEnabled' | 'isGroupCreationEnabled' - > - ) => ({ + (course: CourseData) => ({ label: course.name, value: course.id, isGamified: course.isGamificationEnabled, isGroupCreationEnabled: course.isGroupCreationEnabled, }) ), [dataCourses] )packages/graphql/src/public/client.json (1)
Line range hint
88-145: Summary of changes: Introduction of course archiving featureThe changes in this file consistently support the introduction of a course archiving feature:
- The new
ToggleArchiveCoursemethod allows for toggling the archive status of courses.- The new
GetActiveUserCoursesmethod provides a way to fetch only active (non-archived) courses.- The updated
GetUserCoursesmethod likely now includes information about the archived status of courses.These changes enhance the course management capabilities of the system, allowing for better organization and potentially improved performance when dealing with large numbers of courses.
Consider the following architectural implications:
- Ensure that the archiving feature is consistently applied across all relevant parts of the system (e.g., course listings, search functionality, etc.).
- Update any caching mechanisms to take into account the new archived status of courses.
- Consider adding appropriate access controls to the archiving functionality if it should be restricted to certain user roles.
- Ensure that archiving a course doesn't break any existing functionalities or dependencies (e.g., enrolled students, linked content, etc.).
packages/graphql/src/schema/query.ts (1)
186-192: LGTM! Consider adding an explicit null check for consistency.The new
getActiveUserCoursesquery field is well-implemented and follows the existing patterns in the file. It's properly authenticated and correctly typed.For consistency with some other query resolvers in this file, consider adding an explicit null check:
getActiveUserCourses: asUser.field({ nullable: true, type: [Course], resolve(_, __, ctx) { + if (!ctx.user?.sub) return null return CourseService.getActiveUserCourses(ctx) }, }),This change would align the implementation with queries like
selfanduserProfile, providing an extra layer of safety.cypress/cypress/e2e/E-course-workflow.cy.ts (2)
452-463: Consider adding more assertions for the archiving process.The test correctly checks that the archive button is disabled for running courses and enabled for past courses. However, it could benefit from additional assertions:
- Verify that the confirmation modal contains the correct course name.
- Assert that a success message is displayed after archiving the course.
- Check if the course is removed from the active course list after archiving.
Here's a suggestion to enhance the assertions:
cy.get('[data-cy="course-archive-modal-confirm"]').click() +cy.findByText(messages.manage.courseList.courseArchivedSuccess).should('exist') cy.get(`[data-cy="course-list-button-${pastCourse}"]`).should('not.exist') +cy.get(`[data-cy="course-list-button-${runningCourse}"]`).should('exist')
465-475: Enhance test coverage for archive view and course reactivation.The test covers toggling the archive view and reactivating an archived course. However, it could be improved by:
- Verifying that the running course is not visible in the archive view.
- Checking if the reactivated course appears in the active course list after unarchiving.
- Asserting that a success message is displayed after unarchiving the course.
Consider adding these assertions:
cy.get('[data-cy="toggle-course-archive"]').click() cy.get(`[data-cy="course-list-button-${pastCourse}"]`).should('exist') +cy.get(`[data-cy="course-list-button-${runningCourse}"]`).should('not.exist') cy.get(`[data-cy="archive-course-${pastCourse}"]`).click() cy.findByText(messages.manage.courseList.confirmCourseUnarchive).should( 'exist' ) cy.get('[data-cy="course-archive-modal-confirm"]').click() +cy.findByText(messages.manage.courseList.courseUnarchivedSuccess).should('exist') cy.get('[data-cy="toggle-course-archive"]').click() cy.get(`[data-cy="course-list-button-${pastCourse}"]`).should('exist') +cy.get(`[data-cy="course-list-button-${runningCourse}"]`).should('exist')packages/graphql/src/services/courses.ts (3)
472-490: LGTM: Well-implemented function for toggling course archive status.The
toggleArchiveCoursefunction is well-implemented with proper checks for course ownership and end date. It correctly updates theisArchivedstatus of the course.Consider adding a check to ensure the course exists before attempting to update it. This can be done using
findUniqueOrThrowinstead ofupdate, or by adding a separate existence check before the update operation.Here's a suggested improvement:
export async function toggleArchiveCourse( { id, isArchived }: ToggleArchiveCourseProps, ctx: ContextWithUser ) { - const course = await ctx.prisma.course.update({ + const course = await ctx.prisma.course.findUniqueOrThrow({ where: { id, ownerId: ctx.user.sub, endDate: { lte: new Date(), }, }, + }) + + const updatedCourse = await ctx.prisma.course.update({ + where: { id }, data: { isArchived, }, }) - return course + return updatedCourse }This change ensures that an error is thrown if the course doesn't exist or doesn't meet the criteria, providing better error handling and feedback.
583-595: LGTM: Improved sorting logic for user courses.The changes to
getUserCoursesfunction effectively implement sorting by archived status and start date. The use of optional chaining and nullish coalescing is appropriate.Consider combining the two sorting operations into a single sort for better performance, especially with larger datasets.
Here's a suggested optimization:
export async function getUserCourses(ctx: ContextWithUser) { const userCourses = await ctx.prisma.user.findUnique({ where: { id: ctx.user.sub, }, include: { courses: { orderBy: { createdAt: 'desc', }, }, }, }) - // sort courses by archived or not - const archivedSortedCourses = - userCourses?.courses.sort((a, b) => { - return a.isArchived === b.isArchived ? 0 : a.isArchived ? 1 : -1 - }) ?? [] - - // sort courses by start date descending - const startDateSortedCourses = archivedSortedCourses.sort((a, b) => { - return a.startDate > b.startDate ? -1 : a.startDate < b.startDate ? 1 : 0 - }) - - return startDateSortedCourses + return userCourses?.courses.sort((a, b) => { + if (a.isArchived !== b.isArchived) { + return a.isArchived ? 1 : -1; + } + return b.startDate.getTime() - a.startDate.getTime(); + }) ?? []; }This change combines both sorting criteria into a single sort operation, which is more efficient and easier to read.
597-616: LGTM: Well-implemented function for retrieving active user courses.The
getActiveUserCoursesfunction effectively retrieves active courses for the user, correctly filtering out archived courses and those that have ended. The use of Prisma queries and optional chaining is appropriate.For consistency with the
getUserCoursesfunction, consider adding sorting logic to this function as well.Here's a suggested improvement for consistency:
export async function getActiveUserCourses(ctx: ContextWithUser) { const userCourses = await ctx.prisma.user.findUnique({ where: { id: ctx.user.sub, }, include: { courses: { where: { endDate: { gte: new Date(), }, isArchived: false, }, orderBy: { - createdAt: 'desc', + startDate: 'desc', }, }, }, }) - return userCourses?.courses ?? [] + return userCourses?.courses.sort((a, b) => { + return b.startDate.getTime() - a.startDate.getTime(); + }) ?? []; }This change adds sorting by start date, consistent with the
getUserCoursesfunction, ensuring a uniform ordering of courses across different retrieval methods.packages/graphql/src/schema/mutation.ts (3)
1017-1027: LGTM! Consider adding a description for consistency.The new
toggleArchiveCoursemutation looks good. It's well-structured and uses appropriate authentication. For consistency with other mutations, consider adding a description using thedescriptionfield.You could add a description like this:
toggleArchiveCourse: t.withAuth(asUser).field({ + description: 'Toggle the archived status of a course', nullable: true, type: Course, args: { id: t.arg.string({ required: true }), isArchived: t.arg.boolean({ required: true }), }, resolve(_, args, ctx) { return CourseService.toggleArchiveCourse(args, ctx) }, }),
Line range hint
1028-1041: Address the FIXME comment fortoggleIsArchivedreturn typeThe current implementation uses a type cast to
any, which bypasses TypeScript's type checking. This can lead to potential runtime errors and reduces code maintainability. Consider the following options to improve type safety:
- Create a new Pothos type that represents the partial
Elementreturned by this mutation.- Use a Pick or Omit utility type to create a partial type based on the
Elementtype.- If the exact shape of the returned data is known, define an interface for it.
Here's an example of how you could implement option 2:
type PartialElement = Pick<Element, 'id' | 'isArchived'> // Add other relevant fields toggleIsArchived: t.withAuth(asUserFullAccess).field({ nullable: true, type: [PartialElement], // Use the new type here args: { questionIds: t.arg.intList({ required: true }), isArchived: t.arg.boolean({ required: true }), }, resolve(_, args, ctx) { return QuestionService.toggleIsArchived(args, ctx) }, }),This approach provides better type safety while still allowing flexibility in the returned data structure.
Line range hint
1-1441: Consider improving overall code organization and documentationThe file is well-structured and consistently uses authentication for mutations. However, consider the following suggestions to further improve the code:
Group related mutations: Organize mutations into logical groups (e.g., user operations, course operations, session operations) using comments or by extracting them into separate files.
Add descriptions: Consider adding descriptions to all mutations for better documentation and GraphQL schema introspection.
Use enums for string literals: For arguments that accept specific string values, consider using enums to improve type safety and developer experience.
Consider input types: For mutations with multiple arguments, consider creating input types to group related arguments and improve schema clarity.
Consistent naming: Ensure consistent naming conventions across all mutations (e.g.,
toggleArchiveCoursevstoggleIsArchived).Here's an example of how you could implement some of these suggestions:
// User operations t.group('User', { changeUserLocale: t.withAuth(asUser).field({ description: 'Change the locale for the current user', // ... rest of the mutation }), // ... other user mutations }) // Course operations t.group('Course', { toggleArchiveCourse: t.withAuth(asUser).field({ description: 'Toggle the archived status of a course', // ... rest of the mutation }), // ... other course mutations }) // Example of using an input type const UpdateCourseSettingsInput = builder.inputType('UpdateCourseSettingsInput', { fields: (t) => ({ id: t.string({ required: true }), name: t.string(), displayName: t.string(), // ... other fields }), }) t.field({ // ... args: { input: t.arg({ type: UpdateCourseSettingsInput, required: true }), }, // ... })These changes would make the schema more organized, self-documenting, and easier to maintain.
packages/i18n/messages/de.ts (1)
212-212: Consider additional contexts for "archived" translation.While the addition of the "archived" translation is correct, it might be beneficial to consider if this term will be used in different contexts throughout the application. For example, you might need variations like "Archive" (as a verb) or "Archiving" (as a process) in other parts of the UI. Consider reviewing the application's requirements to ensure all necessary variations of this term are covered.
packages/graphql/src/ops.schema.json (2)
12861-12905: LGTM! Consider adding a description to the query.The new
toggleArchiveCoursequery is well-structured and aligns with the PR objective. The argument types and return type are appropriate for the operation.Consider adding a description to the query to provide more context. For example:
{ "name": "toggleArchiveCourse", - "description": null, + "description": "Toggles the archived status of a course", "args": [ // ... (args remain unchanged) ], // ... (rest of the query definition) }
16216-16235: LGTM! Consider adding a description and pagination.The new
getActiveUserCoursesquery is well-structured and complements the course archiving feature. The return type is appropriate for fetching a list of active courses.Consider the following improvements:
- Add a description to provide more context:
{ "name": "getActiveUserCourses", - "description": null, + "description": "Retrieves a list of active (non-archived) courses for the current user", "args": [], // ... (rest of the query definition) }
- Consider adding pagination to the query to handle potentially large lists of courses:
{ "name": "getActiveUserCourses", "description": "Retrieves a list of active (non-archived) courses for the current user", - "args": [], + "args": [ + { + "name": "first", + "description": "Number of courses to return", + "type": { + "kind": "SCALAR", + "name": "Int", + "ofType": null + }, + "defaultValue": null + }, + { + "name": "after", + "description": "Cursor for pagination", + "type": { + "kind": "SCALAR", + "name": "String", + "ofType": null + }, + "defaultValue": null + } + ], "type": { - "kind": "LIST", + "kind": "OBJECT", + "name": "CourseConnection", "name": null, - "ofType": { - "kind": "NON_NULL", - "name": null, - "ofType": { - "kind": "OBJECT", - "name": "Course", - "ofType": null - } - } }, // ... (rest of the query definition) }This change would require defining a new
CourseConnectiontype withedges,nodes, andpageInfofields, following the Relay connection specification.apps/frontend-manage/src/components/courses/CourseListButton.tsx (1)
59-60: Remove unnecessary.toString()calls afterdayjs().format()The
format()method ofdayjsreturns a string, so calling.toString()is redundant. Removing the.toString()calls will clean up the code without affecting functionality.Apply this diff to remove the redundant calls:
- {dayjs(startDate).format('DD.MM.YYYY').toString()} -{' '} + {dayjs(startDate).format('DD.MM.YYYY')} -{' '} - {dayjs(endDate).format('DD.MM.YYYY').toString()} + {dayjs(endDate).format('DD.MM.YYYY')}apps/frontend-manage/src/pages/courses/index.tsx (2)
103-115: Sanitizecourse.namewhen using in data attributesUsing
course.namedirectly in data attributes might lead to issues if the name contains spaces or special characters, affecting selectors in testing tools.Consider sanitizing the course name:
-data={{ cy: `archive-course-${course.name}` }} +data={{ cy: `archive-course-${course.name.replace(/\s+/g, '-').toLowerCase()}` }}This replaces spaces with hyphens and converts the string to lowercase, resulting in safer attribute values.
35-39: Consistent naming conventions for modal state handlersFor better readability and maintainability, consider standardizing your modal state variable naming conventions.
Currently, the naming is:
createCourseModalandshowCreateCourseModalarchiveModalandshowArchiveModalConsider renaming
showCreateCourseModalandshowArchiveModaltosetCreateCourseModalOpenandsetArchiveModalOpento clearly indicate that they are state setters for modal visibility.Alternatively, you can use a consistent pattern like
[isModalOpen, setIsModalOpen]for all modals.apps/frontend-manage/src/components/courses/modals/CourseManipulationModal.tsx (3)
Line range hint
74-93: Handle potential undefined values inmaxGroupSizevalidationIn the validation schema for
maxGroupSize, themoreThanvalidator comparesmaxGroupSizewithpreferredGroupSize:maxGroupSize: yup .number() .min(2, t('manage.courseList.maxGroupSizeMin')) .moreThan( yup.ref('preferredGroupSize'), t('manage.courseList.maxGroupSizeLargerThanPreferred') ) .required(t('manage.courseList.maxGroupSizeReq')),If
preferredGroupSizeisundefined, themoreThanvalidation may not behave as expected and could cause a validation error.Consider adding a condition to only apply the
moreThanvalidator whenpreferredGroupSizeis defined:maxGroupSize: yup .number() .min(2, t('manage.courseList.maxGroupSizeMin')) + .when('preferredGroupSize', (preferredGroupSize, schema) => + preferredGroupSize + ? schema.moreThan( + yup.ref('preferredGroupSize'), + t('manage.courseList.maxGroupSizeLargerThanPreferred') + ) + : schema + ) .required(t('manage.courseList.maxGroupSizeReq')),This adjustment ensures that the validation only compares the two sizes when both are provided.
Line range hint
56-66: SimplifygroupCreationDeadlinevalidation logicThe validation for
groupCreationDeadlineuses a conditional based oninitialValues?.groupDeadlineDate:groupCreationDeadline: initialValues?.groupDeadlineDate ? yup .date() .min( yup.ref('startDate'), t('manage.courseList.groupDeadlineAfterStart') ) .max( yup.ref('endDate'), t('manage.courseList.groupDeadlineBeforeEnd') ) .required(t('manage.courseList.groupDeadlineReq')) : yup .date() .min(new Date(), t('manage.courseList.groupDeadlineFuture')) .max( yup.ref('endDate'), t('manage.courseList.groupDeadlineBeforeEnd') ) .required(t('manage.courseList.groupDeadlineReq')),This conditional could be simplified to improve readability and maintainability.
Consider refactoring the validation to reduce redundancy:
groupCreationDeadline: yup .date() + .when([], { + is: () => initialValues?.groupDeadlineDate, + then: (schema) => + schema.min( + yup.ref('startDate'), + t('manage.courseList.groupDeadlineAfterStart') + ), + otherwise: (schema) => + schema.min(new Date(), t('manage.courseList.groupDeadlineFuture')), + }) .max( yup.ref('endDate'), t('manage.courseList.groupDeadlineBeforeEnd') ) .required(t('manage.courseList.groupDeadlineReq')),This refactoring removes duplication and makes the validation logic clearer.
Line range hint
61-66: EnsureendDatevalidation handles past dates appropriatelyThe validation for
endDatechecks ifendDatePastis true:endDate: endDatePast ? yup.date() : yup .date() .min(new Date(), t('manage.courseList.endDateFuture')) .min(yup.ref('startDate'), t('manage.courseList.endAfterStart')) .required(t('manage.courseList.courseEndReq')),If
endDatePastis true, the validation becomesyup.date()with no additional constraints, which may allow for invalid or unintended values.To ensure
endDatealways adheres to the necessary constraints, consider adjusting the validation:endDate: yup .date() .min(yup.ref('startDate'), t('manage.courseList.endAfterStart')) + .when([], { + is: () => !endDatePast, + then: (schema) => + schema.min(new Date(), t('manage.courseList.endDateFuture')), + }) .required(t('manage.courseList.courseEndReq')),This ensures that
endDateis always afterstartDateand, if it's not in the past, also in the future.packages/graphql/src/ops.ts (6)
893-896: Ensure Consistent Naming for MutationsThe newly added mutation names
toggleArchiveCourseandtoggleIsArchivedshould reflect their actions clearly and consistently. Consider renamingtoggleIsArchivedtotoggleArchiveQuestionsfor better clarity, since it operates on questions rather than a generic "isArchived" state.
1797-1800: Consistency in Query Naming ConventionsThe addition of the
getActiveUserCoursesquery should follow the existing naming conventions. Consider simplifying the name toactiveUserCoursesto maintain consistency with other queries in the codebase.
3125-3132: Remove Redundant Empty LinesThere are unnecessary empty lines between type definitions that can be removed to improve code readability.
Apply this diff to remove the redundant lines:
export type ToggleArchiveCourseMutationVariables = Exact<{ id: Scalars['String']['input']; isArchived: Scalars['Boolean']['input']; }>; - - export type ToggleArchiveCourseMutation = { __typename?: 'Mutation', toggleArchiveCourse?: { __typename?: 'Course', id: string, isArchived: boolean } | null };
Line range hint
3734-3741: Format Long JSON Definitions for Improved ReadabilityThe JSON definitions for GraphQL operations are on single lines, which can be hard to read and maintain. Consider formatting the JSON objects across multiple lines.
Example refactor:
-export const ToggleArchiveCourseDocument = {"kind":"Document","definitions":[{"kind":"OperationDefinition","operation":"mutation","name":{"kind":"Name","value":"ToggleArchiveCourse"},"variableDefinitions":[{"kind":"VariableDefinition","variable":{"kind":"Variable","name":{"kind":"Name","value":"id"}},"type":{"kind":"NonNullType","type":{"kind":"NamedType","name":{"kind":"Name","value":"String"}}}},{"kind":"VariableDefinition","variable":{"kind":"Variable","name":{"kind":"Name","value":"isArchived"}},"type":{"kind":"NonNullType","type":{"kind":"NamedType","name":{"kind":"Name","value":"Boolean"}}}}],"selectionSet":{"kind":"SelectionSet","selections":[{"kind":"Field","name":{"kind":"Name","value":"toggleArchiveCourse"},"arguments":[{"kind":"Argument","name":{"kind":"Name","value":"id"},"value":{"kind":"Variable","name":{"kind":"Name","value":"id"}}},{"kind":"Argument","name":{"kind":"Name","value":"isArchived"},"value":{"kind":"Variable","name":{"kind":"Name","value":"isArchived"}}}],"selectionSet":{"kind":"SelectionSet","selections":[{"kind":"Field","name":{"kind":"Name","value":"id"}},{"kind":"Field","name":{"kind":"Name","value":"isArchived"}}]}}]}}]} as unknown as DocumentNode<ToggleArchiveCourseMutation, ToggleArchiveCourseMutationVariables>; +export const ToggleArchiveCourseDocument = { + "kind": "Document", + "definitions": [ + { + "kind": "OperationDefinition", + "operation": "mutation", + "name": { "kind": "Name", "value": "ToggleArchiveCourse" }, + "variableDefinitions": [ + { + "kind": "VariableDefinition", + "variable": { "kind": "Variable", "name": { "kind": "Name", "value": "id" } }, + "type": { "kind": "NonNullType", "type": { "kind": "NamedType", "name": { "kind": "Name", "value": "String" } } } + }, + { + "kind": "VariableDefinition", + "variable": { "kind": "Variable", "name": { "kind": "Name", "value": "isArchived" } }, + "type": { "kind": "NonNullType", "type": { "kind": "NamedType", "name": { "kind": "Name", "value": "Boolean" } } } + } + ], + "selectionSet": { + "kind": "SelectionSet", + "selections": [ + { + "kind": "Field", + "name": { "kind": "Name", "value": "toggleArchiveCourse" }, + "arguments": [ + { "kind": "Argument", "name": { "kind": "Name", "value": "id" }, "value": { "kind": "Variable", "name": { "kind": "Name", "value": "id" } } }, + { "kind": "Argument", "name": { "kind": "Name", "value": "isArchived" }, "value": { "kind": "Variable", "name": { "kind": "Name", "value": "isArchived" } } } + ], + "selectionSet": { + "kind": "SelectionSet", + "selections": [ + { "kind": "Field", "name": { "kind": "Name", "value": "id" } }, + { "kind": "Field", "name": { "kind": "Name", "value": "isArchived" } } + ] + } + } + ] + } + } + ] +} as unknown as DocumentNode<ToggleArchiveCourseMutation, ToggleArchiveCourseMutationVariables>;
Line range hint
3751-3755: Include 'owner' Field in 'GetActiveUserCourses' QueryConsider including the
ownerfield in theGetActiveUserCoursesQueryto provide additional context about who owns each course. This can be useful for frontend components that display course information.
Line range hint
3791-3797: Duplicate Query Definition DetectedThe
GetUserCoursesQueryappears to be defined more than once. Ensure that duplicate definitions are removed to prevent confusion and potential redundancy.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (20)
- apps/frontend-manage/src/components/courses/CourseListButton.tsx (2 hunks)
- apps/frontend-manage/src/components/courses/modals/CourseArchiveModal.tsx (1 hunks)
- apps/frontend-manage/src/components/courses/modals/CourseManipulationModal.tsx (1 hunks)
- apps/frontend-manage/src/components/sessions/creation/ElementCreation.tsx (2 hunks)
- apps/frontend-manage/src/pages/courses/index.tsx (4 hunks)
- cypress/cypress/e2e/E-course-workflow.cy.ts (1 hunks)
- packages/graphql/src/graphql/ops/MToggleArchiveCourse.graphql (1 hunks)
- packages/graphql/src/graphql/ops/QGetActiveUserCourses.graphql (1 hunks)
- packages/graphql/src/graphql/ops/QGetUserCourses.graphql (1 hunks)
- packages/graphql/src/ops.schema.json (2 hunks)
- packages/graphql/src/ops.ts (9 hunks)
- packages/graphql/src/public/client.json (3 hunks)
- packages/graphql/src/public/schema.graphql (2 hunks)
- packages/graphql/src/public/server.json (3 hunks)
- packages/graphql/src/schema/mutation.ts (1 hunks)
- packages/graphql/src/schema/query.ts (1 hunks)
- packages/graphql/src/services/courses.ts (2 hunks)
- packages/i18n/messages/de.ts (2 hunks)
- packages/i18n/messages/en.ts (2 hunks)
- packages/prisma/src/data/seedTEST.ts (2 hunks)
🔇 Additional comments (30)
apps/frontend-manage/src/components/courses/modals/CourseArchiveModal.tsx (1)
1-14: LGTM: Imports and interface definition are well-structured.The imports cover all necessary dependencies for GraphQL operations, UI components, and internationalization. The
CourseArchiveModalPropsinterface is clearly defined with appropriate types for each prop.apps/frontend-manage/src/components/sessions/creation/ElementCreation.tsx (2)
108-108: LGTM! Query usage updated correctly.The query usage has been correctly updated to use
GetActiveUserCoursesDocument. This change is consistent with the import modification and ensures that only active user courses are fetched.
Line range hint
1-264: Overall, the changes look good. Verify the impact on the user experience.The modifications to fetch and use active user courses have been implemented correctly and consistently throughout the file. These changes align well with the PR objectives.
To ensure these changes don't negatively impact the user experience, please verify the following:
- Confirm that users can still access all necessary courses in the UI.
- Check if any UI components or user flows need to be updated to reflect the concept of "active" courses.
- Ensure that the documentation is updated to reflect this change in course fetching behavior.
Consider running integration tests or conducting a manual QA session to validate these points.
packages/graphql/src/public/client.json (3)
145-145: Updated hash value for GetUserCoursesThe hash value for the
GetUserCoursesmethod has been updated, which indicates a change in the query implementation. This change is likely related to the new archiving feature, possibly to include anisArchivedfield in the course data.Let's verify the changes made to the
GetUserCoursesquery:#!/bin/bash # Search for GetUserCourses query definition rg "query GetUserCourses" --type graphql -A 10
105-105: New method added: GetActiveUserCoursesThis new method suggests the implementation of a query to fetch only active (non-archived) courses for a user. This is a useful addition that complements the new archiving feature, allowing for more efficient course management and display.
Let's verify the implementation and usage of this new method:
#!/bin/bash # Search for GetActiveUserCourses usage rg "GetActiveUserCourses" --type ts --type tsx
88-88: New method added: ToggleArchiveCourseThis new method introduces the ability to toggle the archive status of a course. This is a valuable feature for course management, allowing users to hide or unhide courses as needed.
To ensure this new method is properly implemented and used, let's verify its usage across the codebase:
✅ Verification successful
Usage of ToggleArchiveCourse Verified
The
ToggleArchiveCoursemethod is correctly utilized across the codebase in the following locations:
packages/graphql/src/services/courses.tspackages/graphql/src/ops.tsapps/frontend-manage/src/components/courses/modals/CourseArchiveModal.tsxNo issues found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for ToggleArchiveCourse usage rg "ToggleArchiveCourse" --type ts --type tsxLength of output: 150
Script:
#!/bin/bash # Search for ToggleArchiveCourse usage in TypeScript and TSX files rg "ToggleArchiveCourse" --type ts --type tsxLength of output: 73
Script:
#!/bin/bash # Search for ToggleArchiveCourse usage in TypeScript and TSX files rg "ToggleArchiveCourse" --glob "*.ts" --glob "*.tsx"Length of output: 2077
cypress/cypress/e2e/E-course-workflow.cy.ts (2)
440-450: LGTM: Test setup and initial checks are well-structured.The test case begins with a clear setup, defining course names and logging in as a lecturer. It also verifies the existence of both running and past courses, which is a good initial check.
Line range hint
1-475: Overall, the new test case enhances the test suite.The addition of the "Test archiving and deleting courses" test case is a valuable improvement to the E2E test suite. It covers important functionality related to course management and integrates well with the existing tests. While there are opportunities for further enhancement, as noted in the previous comments, the overall changes are positive and contribute to better test coverage.
packages/graphql/src/services/courses.ts (1)
467-470: LGTM: Well-defined interface for toggling course archive status.The
ToggleArchiveCoursePropsinterface is clear, concise, and follows TypeScript best practices. It provides the necessary properties for toggling the archive status of a course.packages/graphql/src/public/server.json (3)
Line range hint
88-92: LGTM: New mutation for toggling course archive statusThe new
ToggleArchiveCoursemutation is well-structured and follows GraphQL best practices. It appropriately takes the courseidand desiredisArchivedstatus as input, and returns the updated fields.
Line range hint
105-121: LGTM: New query for retrieving active user coursesThe
GetActiveUserCoursesquery is well-designed and provides a comprehensive set of fields for active user courses. It includes all necessary information such as course details, gamification settings, and important dates.
Line range hint
145-157: LGTM: EnhancedGetUserCoursesquery with additional fieldsThe
GetUserCoursesquery has been improved with the addition ofcolor,startDate,endDate,createdAt, andupdatedAtfields. These enhancements provide more comprehensive information about user courses and align well with the newly addedGetActiveUserCoursesquery.packages/i18n/messages/en.ts (5)
212-212: New translation added for 'archived' statusThe addition of the 'archived' translation in the generic section is appropriate. It provides a clear and concise label for archived items, which is consistent with the new archiving functionality being implemented.
1500-1501: New translations for archive/unarchive course actionsThe additions of 'archiveCourse' and 'unarchiveCourse' translations are clear and concise. They accurately describe the actions that can be performed on a course.
1502-1505: Confirmation messages for archive/unarchive actionsThe 'confirmCourseArchive' and 'confirmCourseUnarchive' messages provide clear explanations of the archiving and unarchiving actions. They inform the user about the consequences of these actions, which is good for user understanding and preventing accidental actions.
1506-1507: Toggle translations for showing/hiding archived coursesThe 'showArchive' and 'hideArchive' translations are clear and concise. They provide appropriate labels for toggling the visibility of archived courses in the user interface.
212-212: LGTM! Well-implemented translations for course archiving featureThe new translations added for the course archiving feature are clear, concise, and consistent with the existing style of the application. They provide appropriate labels and informative messages for users interacting with the archive functionality. The additions cover all necessary aspects of the feature, including toggling archive visibility, performing archive/unarchive actions, and confirming these actions. These translations will contribute to a good user experience when working with archived courses.
Also applies to: 1500-1507
packages/i18n/messages/de.ts (2)
212-212: LGTM: New translation added correctly.The new translation for "archived" has been added correctly to the
genericsection of the German localization file. It follows the existing pattern and naming conventions used throughout the file.
Line range hint
1-1526: Overall LGTM: File structure and consistency maintained.The addition of the "archived" translation maintains the file's structure and consistency. The change is minimal and doesn't introduce any apparent issues. The file continues to provide comprehensive German translations for various parts of the KlickerUZH application.
packages/graphql/src/ops.schema.json (1)
Line range hint
12861-16235: Overall, the schema changes look good and support the new course archiving feature.The additions to the GraphQL schema, including the
toggleArchiveCourseandgetActiveUserCoursesqueries, are well-structured and align with the PR objective. The suggested improvements (adding descriptions and considering pagination) will enhance the API's usability and scalability.apps/frontend-manage/src/components/courses/CourseListButton.tsx (2)
41-44: Ensure conditional class application works as intendedThe conditional application of the
'!border-b-4'class using:typeof color !== 'undefined' && '!border-b-4'Ensure that this logic correctly adds the class when
coloris defined. Also, verify thattwMergehandles the class merging appropriately, especially with the important modifier (!).
65-65: Verify that the translation key'shared.generic.archived'exists in all localesEnsure that the translation key used in:
{t('shared.generic.archived')}is present in all locale files to avoid missing translations.
Run the following script to check for the existence of the translation key:
apps/frontend-manage/src/pages/courses/index.tsx (1)
114-114:⚠️ Potential issueEnsure
course.endDateis notnullorundefinedThe expression
dayjs(course.endDate).isAfter(dayjs())assumes thatcourse.endDateis always defined. Ifcourse.endDatecan benullorundefined, this may lead to runtime errors.Consider adding a null check to handle cases where
course.endDateis not set:-disabled={dayjs(course.endDate).isAfter(dayjs())} +disabled={course.endDate ? dayjs(course.endDate).isAfter(dayjs()) : true}This ensures the button is disabled if the course has not ended or if the end date is unknown.
Likely invalid or redundant comment.
apps/frontend-manage/src/components/courses/modals/CourseManipulationModal.tsx (1)
142-142: Verify Modal'sclassNamechanges for responsive designThe addition of
className={{ content: 'h-max !w-full' }}to theModalcomponent modifies the content area's height and width by setting it to maximum height and forcing full width with!w-full. The use of the!prefix overrides any previous width utilities, which might lead to unintended styling issues on various screen sizes.Please ensure that this change does not adversely affect the modal's responsiveness. You can verify the impact by testing the modal on different viewport sizes.
packages/graphql/src/public/schema.graphql (1)
1062-1062: 🛠️ Refactor suggestionEnsure consistency with existing course retrieval queries
The query
getActiveUserCoursesretrieves active courses for the user. Please ensure that this query does not duplicate existing functionality, such asparticipantCoursesoruserCourses.Recommendation:
- If
getActiveUserCoursesprovides distinct functionality (e.g., filtering out archived courses), consider renaming it to clarify its purpose, such asgetNonArchivedUserCoursesor adding a filter parameter to existing queries.- Review existing queries to determine if this functionality can be integrated without introducing a new method.
packages/prisma/src/data/seedTEST.ts (4)
131-132: Course name updated appropriatelyThe
nameanddisplayNamehave been changed to'Testkurs 2', which is consistent and clear.
136-136: Course color updated to '#ff0000'The new color code is valid and the update is acceptable.
156-156: Course color updated to '#166b16'The color code update is valid and acceptable.
139-139:⚠️ Potential issueEnd date is in the past
The
endDateis set to'2024-01-01T23:59', which is in the past considering the current date is October 2024. Please verify if this is intentional or update the date to a future point.packages/graphql/src/ops.ts (1)
1465-1469: Verify Mutation Argument Types AlignmentEnsure that the argument types for
MutationToggleArchiveCourseArgsmatch the expected types defined in the GraphQL schema. Specifically, confirm that theidis correctly typed asString!andisArchivedasBoolean!.Run the following script to verify the mutation argument types:
apps/frontend-manage/src/components/courses/modals/CourseArchiveModal.tsx
Show resolved
Hide resolved
apps/frontend-manage/src/components/sessions/creation/ElementCreation.tsx
Show resolved
Hide resolved
No description provided.