enhance: combine scheduling logic for practice quizzes into publication logic #4374
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.
|
📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces a new property, Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🔇 Additional comments (2)packages/i18n/messages/de.ts (2)
The translation strings are clear and consistent with the application's validation message style.
The translations effectively support the enhanced publication workflow with:
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (10)
apps/frontend-manage/src/components/courses/PracticeQuizList.tsx (1)
12-12: Consider using Date type for courseStartDateUsing a string type for dates can lead to inconsistencies in date handling and validation. Consider using the Date type for better type safety and date manipulation capabilities.
- courseStartDate: string + courseStartDate: Dateapps/frontend-manage/src/components/courses/actions/PublishPracticeQuizButton.tsx (1)
11-15: Consider using a more specific type for courseStartDateWhile the type narrowing for
practiceQuizusingPickis excellent, thecourseStartDatetype could be more specific to ensure date format consistency.Consider using a more specific type:
- courseStartDate: string + courseStartDate: `${number}-${number}-${number}` // YYYY-MM-DD formatOr add runtime validation:
function isValidDateFormat(date: string): boolean { return /^\d{4}-\d{2}-\d{2}$/.test(date) && !isNaN(Date.parse(date)) }apps/frontend-manage/src/components/courses/modals/PublishConfirmationModal.tsx (2)
Line range hint
54-61: Consider adding error handling.While the logic is correct, consider adding error handling to catch and process potential mutation failures gracefully.
onClick={async () => { + try { if (elementType === ElementInstanceType.Microlearning) { await publishMicroLearning() } else if (elementType === ElementInstanceType.GroupActivity) { await publishGroupActivity() } setOpen(false) + } catch (error) { + // Handle error (e.g., show error message) + console.error('Failed to publish:', error) + } }}
Line range hint
1-99: Consider enhancing modal reusability.The modal component could be made more reusable by:
- Extracting the mutation logic into a custom hook
- Using a strategy pattern for different element types
- Making the modal more generic with pluggable actions
This would make it easier to add support for new element types in the future without modifying the modal component itself.
apps/frontend-manage/src/components/courses/modals/PracticeQuizPublishingModal.tsx (1)
32-32: Address the TODO comment for translations.The comment indicates that translations are missing for the component's content.
Would you like me to help generate the translation keys and their corresponding English values for this component?
apps/frontend-manage/src/components/sessions/creation/practiceQuiz/PracticeQuizSettingsStep.tsx (1)
163-170: Enhance accessibility and user guidanceWhile the warning message about moved scheduling functionality is well-implemented, consider these improvements:
- Add ARIA attributes for better accessibility
- Provide more specific guidance about where to find the scheduling functionality
Consider applying these changes:
<div className="flex flex-row items-center gap-4"> <FontAwesomeIcon icon={faWarning} className="text-orange-500" + aria-hidden="true" /> - <div className="mt-1 text-sm"> + <div className="mt-1 text-sm" role="alert" aria-live="polite"> {t('manage.sessionForms.practiceQuizSchedulingMoved')} </div> </div>Also, consider updating the translation key 'manage.sessionForms.practiceQuizSchedulingMoved' to include information about where users can now find the scheduling functionality (e.g., in the publication flow).
apps/frontend-manage/src/pages/courses/[id].tsx (1)
Line range hint
248-275: Consider standardizing date prop handling across componentsTo improve maintainability and consistency, consider:
- Creating a common interface for date-related props
- Documenting the expected date format and timezone handling
- Adding prop-types or TypeScript interfaces to enforce consistent usage
This will help ensure consistent behavior across all components that handle course dates.
packages/graphql/src/public/schema.graphql (1)
1028-1029: LGTM! Added optional availableFrom parameter to publishPracticeQuiz.The addition of an optional
availableFromparameter to the publication mutation provides flexibility in scheduling:
- Immediate publication when
availableFromis not provided- Scheduled publication when a future date is provided
This change follows the principle of separation of concerns by:
- Separating content management (create/edit) from publication workflow
- Consolidating scheduling logic into the publication phase
- Providing a more intuitive API where availability is determined during publication
packages/graphql/src/ops.schema.json (1)
15067-15069: Add description for the availableFrom parameter.Consider adding a description to document the parameter's purpose and behavior, especially since this is part of a significant change in how scheduling works.
{ "name": "availableFrom", - "description": null, + "description": "Optional date when the practice quiz should become available. If not provided, the quiz will be available immediately upon publication.", "type": {packages/graphql/src/services/practiceQuizzes.ts (1)
2989-2996: Simplify theavailableFromparameter typeSince
availableFromis optional and can be omitted, you can simplify its type by removing| null, changing it toavailableFrom?: Date. This avoids allowingnullwhen it's not necessary.Apply this diff to simplify the parameter type:
export async function publishPracticeQuiz( { id, availableFrom, }: { id: string - availableFrom?: Date | null + availableFrom?: Date }, ctx: ContextWithUser ) {
🛑 Comments failed to post (4)
apps/frontend-manage/src/components/courses/modals/PracticeQuizPublishingModal.tsx (2)
46-53: 🛠️ Refactor suggestion
Move hard-coded strings to translation keys.
Several UI strings are hard-coded in English. These should be moved to translation keys for internationalization support:
- "Publish Immediately"
- The entire explanation text about immediate publication
- "Confirm Publication"
Example refactor:
-<H3 className={{ root: 'mb-0' }}>Publish Immediately</H3> +<H3 className={{ root: 'mb-0' }}>{t('practiceQuiz.publishImmediately')}</H3>Also applies to: 73-73
80-88: 🛠️ Refactor suggestion
Move remaining hard-coded strings to translation keys.
Additional strings that need translation:
- "Schedule Publication"
- The explanation text about scheduled publication
- "Confirm Scheduling"
Also applies to: 140-140
packages/i18n/messages/en.ts (1)
1166-1167:
⚠️ Potential issueFix typo in the translation key name.
The key name contains a typo:
practiceQuizStartReqeuiredshould bepracticeQuizStartRequired.Apply this diff to fix the typo:
- practiceQuizStartReqeuired: + practiceQuizStartRequired: 'Please choose the start date of the practice quiz.',📝 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.practiceQuizStartRequired: 'Please choose the start date of the practice quiz.',packages/i18n/messages/de.ts (1)
1172-1173:
⚠️ Potential issueFix typo in translation key name.
The key name contains a typo:
practiceQuizStartReqeuiredshould bepracticeQuizStartRequired.Apply this diff to fix the typo:
- practiceQuizStartReqeuired: + practiceQuizStartRequired:📝 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.practiceQuizStartRequired: 'Bitte geben Sie ein Startdatum für Ihr Übungs-Quiz ein.',
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
apps/frontend-manage/src/components/courses/modals/PracticeQuizPublishingModal.tsx (2)
21-31: Consider adding error handling for the mutation.While the mutation setup is good, consider adding error handling to provide feedback to users when publication fails.
const [publishPracticeQuiz, { loading: practiceQuizPublishing }] = - useMutation(PublishPracticeQuizDocument) + useMutation(PublishPracticeQuizDocument, { + onError: (error) => { + // Handle error appropriately (e.g., show toast notification) + console.error('Failed to publish practice quiz:', error) + }, + })
21-143: Consider component decomposition and hook extraction.The component handles multiple responsibilities and could benefit from:
- Splitting into smaller components (ImmediatePublication and ScheduledPublication)
- Moving the mutation logic to a custom hook (e.g.,
usePublishPracticeQuiz)This would improve maintainability and reusability.
🛑 Comments failed to post (3)
apps/frontend-manage/src/components/courses/modals/PracticeQuizPublishingModal.tsx (2)
98-98:
⚠️ Potential issueFix typo in validation message key.
There's a typo in the validation message key: 'practiceQuizStartReqeuired' should be 'practiceQuizStartRequired'.
- .required(t('manage.sessionForms.practiceQuizStartReqeuired')) + .required(t('manage.sessionForms.practiceQuizStartRequired'))📝 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..required(t('manage.sessionForms.practiceQuizStartRequired'))
32-70: 🛠️ Refactor suggestion
Consider handling the mutation response.
The immediate publication button should handle the mutation response to confirm successful publication.
onClick={async () => { - await publishPracticeQuiz({ - variables: { - id: elementId, - }, - }) - setOpen(false) + try { + const response = await publishPracticeQuiz({ + variables: { + id: elementId, + }, + }) + if (response.data?.publishPracticeQuiz) { + // Show success notification + setOpen(false) + } + } catch (error) { + // Error already handled by mutation error handler + } }}Committable suggestion skipped: line range outside the PR's diff.
packages/i18n/messages/de.ts (1)
1172-1173:
⚠️ Potential issueFix typo in key name.
There's a typo in the key name
practiceQuizStartReqeuired(misspelled "Required").Apply this diff to fix the typo:
- practiceQuizStartReqeuired: + practiceQuizStartRequired:📝 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.practiceQuizStartRequired: 'Bitte geben Sie ein Startdatum für Ihr Übungs-Quiz ein.',
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
cypress/cypress/e2e/I-bookmarking-workflow.cy.ts (1)
Line range hint
1-276: Consider these improvements for better test reliability and maintainability
- Replace hardcoded wait times with proper Cypress wait commands:
-cy.wait(500) +cy.waitUntil(() => cy.get('[data-cy="element-state"]').should('have.class', 'updated'))
- Extract common cleanup logic into a custom command:
// In commands.ts Cypress.Commands.add('cleanupActivity', (type, name, courseName) => { cy.get('[data-cy="courses"]').click() cy.get(`[data-cy="course-list-button-${courseName}"]`).click() cy.get(`[data-cy="tab-${type}s"]`).click() cy.get(`[data-cy="${type}-actions-${name}"]`).click() cy.get(`[data-cy="delete-${type}-${name}"]`).click() cy.get(`[data-cy="confirm-deletion-responses"]`).click() cy.get(`[data-cy="activity-confirmation-modal-confirm"]`).click() cy.get(`[data-cy="${type}-actions-${name}"]`).should('not.exist') })
- Consider using before/after hooks for setup and cleanup to improve test organization
cypress/cypress/e2e/H-practice-quiz-workflow.cy.ts (2)
638-654: Enhance test assertions for better clarity.While the test covers both invalid and valid scheduling scenarios, it could be more explicit in its assertions.
Consider adding explicit assertions to verify why the button is disabled:
cy.get('[data-cy="schedule-practice-quiz-publication"]').should( 'be.disabled' ) +cy.get('[data-cy="schedule-error-message"]').should( + 'contain', + 'Publication date cannot be before course start date' +)
686-689: Consider adding additional assertions for immediate publication.The test verifies basic functionality but could benefit from additional assertions to ensure the publication state is correct.
Consider adding these assertions:
cy.get('[data-cy="schedule-practice-quiz-publication"]').click() +// Verify immediate publication state +cy.get('[data-cy="publication-status"]').should('contain', 'Published immediately') +cy.get('[data-cy="publication-date"]').should('contain', `${currentYear - 1}-01-01`) cy.get(`[data-cy="practice-quiz-${scheduledName}"]`).contains( messages.shared.generic.published )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
cypress/cypress/e2e/H-practice-quiz-workflow.cy.ts(4 hunks)cypress/cypress/e2e/I-bookmarking-workflow.cy.ts(1 hunks)cypress/cypress/support/commands.ts(0 hunks)packages/i18n/messages/de.ts(2 hunks)packages/i18n/messages/en.ts(2 hunks)
💤 Files with no reviewable changes (1)
- cypress/cypress/support/commands.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/i18n/messages/en.ts
🔇 Additional comments (5)
cypress/cypress/e2e/I-bookmarking-workflow.cy.ts (1)
84-84: LGTM: Publishing workflow update aligns with PR objectives
The change from a confirmation-based to an immediate publication workflow is consistent with the PR's goal of combining scheduling logic into publication logic.
Let's verify that we're testing all aspects of the new publishing workflow:
✅ Verification successful
Publishing workflow update is consistently implemented
The change from confirmation-based to immediate publication is consistently implemented across both test files:
I-bookmarking-workflow.cy.tsH-practice-quiz-workflow.cy.ts
Both files use the same data-cy selector publish-practice-quiz-immediately for the new immediate publication workflow, and both have comprehensive test coverage for the publishing scenarios including:
- Immediate publication
- Scheduled publication
- Publication state verification
- Student visibility verification
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other test files that might need similar updates for the publishing workflow
# Search for other test files that might contain practice quiz publishing tests
fd -e cy.ts -e cy.js | xargs rg -l "publish.*practice.*quiz"
# Search for the specific data-cy attribute to ensure consistent usage
rg "data-cy=\"publish-practice-quiz-immediately\""
Length of output: 449
Script:
#!/bin/bash
# Let's examine the practice quiz workflow test file to ensure consistent publishing behavior
cat "cypress/cypress/e2e/H-practice-quiz-workflow.cy.ts"
Length of output: 26928
cypress/cypress/e2e/H-practice-quiz-workflow.cy.ts (2)
394-394: LGTM! Test name accurately reflects the purpose.
The renamed test case better describes its intention of testing the scheduling functionality.
494-494: LGTM! Selector name is more descriptive.
The new data-cy selector 'publish-practice-quiz-immediately' better describes the specific action being tested.
packages/i18n/messages/de.ts (2)
1168-1169: LGTM: Clear messaging for practice quiz scheduling changes.
The new strings effectively communicate:
- The relocation of scheduling functionality
- The requirement for specifying a start date
Also applies to: 1172-1173
1755-1762: LGTM: Comprehensive publishing workflow messages.
The new strings provide clear guidance through the publishing workflow:
- Immediate publishing option with explanation
- Scheduled publishing option with explanation
- Confirmation steps for both workflows
klicker-uzh
|
||||||||||||||||||||||||||||
| Project |
klicker-uzh
|
| Branch Review |
enhanced-pq-scheduling
|
| Run status |
|
| Run duration | 11m 19s |
| Commit |
|
| Committer | Julius Schlapbach |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
140
|
| View all changes introduced in this branch ↗︎ | |
…se, remove info that publication date was moved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
apps/frontend-manage/src/components/sessions/creation/CourseSelectionMonitorPracticeQuiz.tsx (2)
28-41: Optimize course finding logicThe current implementation can be made more concise and maintainable using array methods.
Consider this optimization:
- let course = gamifiedCourses?.find( - (course) => course.value === values.courseId - ) - - if (!course) { - course = nonGamifiedCourses?.find( - (course) => course.value === values.courseId - ) - } - - if (!course) { - console.log('Invalid course selection') - return - } + const course = [...(gamifiedCourses ?? []), ...(nonGamifiedCourses ?? [])] + .find((course) => course.value === values.courseId) + + if (!course) { + console.warn(`No course found with ID: ${values.courseId}`) + return + }
Line range hint
27-58: Consider batching state updates and improving error handlingThe current implementation makes multiple sequential state updates which could potentially be batched. Additionally, error handling could be improved for async operations.
Consider these improvements:
useEffect(() => { if (values.courseId) { const course = [...(gamifiedCourses ?? []), ...(nonGamifiedCourses ?? [])] .find((course) => course.value === values.courseId) if (!course) { - console.warn(`No course found with ID: ${values.courseId}`) + console.error(`No course found with ID: ${values.courseId}. Available courses: ${ + [...(gamifiedCourses ?? []), ...(nonGamifiedCourses ?? [])] + .map(c => c.value) + .join(', ') + }`) return } + // Batch state updates + Promise.all([ setCourseGamified(course.isGamified), setTouched({ courseStartDate: true, courseEndDate: true, }), setValues( (prev) => ({ ...prev, courseStartDate: course.startDate, courseEndDate: course.endDate, }), true - ) + ) + ]).catch((error) => { + console.error('Failed to update course state:', error) + }) } }, [values.courseId])apps/frontend-manage/src/components/sessions/creation/practiceQuiz/PracticeQuizSettingsStep.tsx (1)
Line range hint
1-180: Consider splitting the component for better maintainability.The component handles multiple responsibilities (gamification settings, course selection, general settings). Consider splitting it into smaller, focused components:
GamificationSectionGeneralSettingsSectionCourseSelectionSectionThis would improve maintainability and make the code more testable.
Example refactor for the gamification section:
interface GamificationSectionProps { courseGamified: boolean; values: FormikValues; groupedCourses: any; // type this appropriately t: (key: string) => string; } function GamificationSection({ courseGamified, values, groupedCourses, t }: GamificationSectionProps) { return ( <div className={twMerge( 'border-uzh-grey-40 w-full rounded-md border border-solid p-2 shadow-md md:w-72', courseGamified && 'border-orange-400' )}> <div className="flex flex-row items-center justify-center gap-2"> <FontAwesomeIcon icon={faCrown} className="text-orange-400" /> <div className="text-lg font-bold"> {t('shared.generic.gamification')} </div> </div> {/* Rest of the gamification section */} </div> ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
apps/frontend-manage/src/components/sessions/creation/CourseSelectionMonitorPracticeQuiz.tsx(2 hunks)apps/frontend-manage/src/components/sessions/creation/practiceQuiz/PracticeQuizSettingsStep.tsx(1 hunks)
🔇 Additional comments (4)
apps/frontend-manage/src/components/sessions/creation/CourseSelectionMonitorPracticeQuiz.tsx (2)
9-9: Verify props availability to prevent runtime issues
Both gamifiedCourses and nonGamifiedCourses are optional props. Consider adding validation to ensure at least one of these arrays is provided to prevent potential runtime issues.
Consider adding a runtime check:
function CourseSelectionMonitorPracticeQuiz({
values,
gamifiedCourses,
nonGamifiedCourses,
setCourseGamified,
setTouched,
setValues,
}: {
// ... props interface
}) {
+ if (!gamifiedCourses && !nonGamifiedCourses) {
+ console.warn('Neither gamified nor non-gamified courses provided')
+ }Also applies to: 16-16
Line range hint 1-65: Verify component usage across the codebase
Let's ensure that all parent components are updated to pass the new nonGamifiedCourses prop.
✅ Verification successful
Based on the shell script results, I can see that CourseSelectionMonitorPracticeQuiz is only used in one place: PracticeQuizSettingsStep.tsx. The usage shows that the component is already being used correctly with all required props, including the nonGamifiedCourses prop:
<CourseSelectionMonitorPracticeQuiz
values={values}
gamifiedCourses={gamifiedCourses}
nonGamifiedCourses={nonGamifiedCourses}
setCourseGamified={setCourseGamified}
setTouched={setTouched}
setValues={setValues}
/>Component usage is consistent and properly updated
The parent component PracticeQuizSettingsStep is already passing all required props, including the nonGamifiedCourses prop, to CourseSelectionMonitorPracticeQuiz. No further updates are needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of CourseSelectionMonitorPracticeQuiz
ast-grep --pattern 'CourseSelectionMonitorPracticeQuiz' -A 10
# Search for components that might need to pass nonGamifiedCourses
rg -l "CourseSelectionMonitorPracticeQuiz"
Length of output: 4837
apps/frontend-manage/src/components/sessions/creation/practiceQuiz/PracticeQuizSettingsStep.tsx (2)
60-60: LGTM: Prop addition aligns with component requirements.
The addition of nonGamifiedCourses prop to CourseSelectionMonitorPracticeQuiz is consistent with the component's responsibility to handle both gamified and non-gamified courses.
Line range hint 26-180: Verify the impact of removed availability date selection.
The AI summary indicates that availability date selection was removed. This could impact existing features or integrations.
|
Fixed a small bug with the course selection dropdown not accurately updating the "isGamified" state (unrelated to the changes in this PR). Removed the notification, that it has been moved. LGTM :) |
|
klicker-uzh
|
||||||||||||||||||||||||||||
| Project |
klicker-uzh
|
| Branch Review |
v3
|
| Run status |
|
| Run duration | 11m 14s |
| Commit |
|
| Committer | Julius Schlapbach |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
|
Failures
|
0
|
|
Flaky
|
0
|
|
Pending
|
0
|
|
Skipped
|
0
|
|
Passing
|
140
|
| View all changes introduced in this branch ↗︎ | |
Summary by CodeRabbit
Release Notes
New Features
courseStartDateproperty to enhance quiz context across various components.PracticeQuizPublishingModalfor improved quiz publication management, including immediate and scheduled options.Bug Fixes
Documentation
Chores