enhance: implement grading logic and extend frontend components for selection question compatibility #4441
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.
|
️✅ There are no secrets present in this pull request anymore.If these secrets were true positive and are still valid, we highly recommend you to revoke them. 🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request. |
|
Warning Rate limit exceeded@sjschlapbach has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 34 minutes and 42 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis pull request introduces a series of modifications across multiple components and packages, focusing on enhancing the functionality of question manipulation, grading, and selection-based interactions. The changes primarily involve updating interfaces, adding new props, modifying validation schemas, and improving localization for selection-type questions. The modifications span frontend components, shared components, internationalization files, and grading logic, with a particular emphasis on improving the user experience and validation mechanisms for answer selection. Changes
Possibly related PRs
Suggested reviewers
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 (
|
sjschlapbach
changed the title
…s to remain smaller than the number of answer options
![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
🧹 Nitpick comments (9)
apps/frontend-manage/src/pages/questions/[id].tsx (1)
56-72: Consider improving responsiveness and style reusability.The layout structure and visual presentation look good, but there are opportunities for improvement:
- The hardcoded width of
64remmight not be optimal for all screen sizes.- Common styles could be extracted to maintain consistency across similar components.
Consider these improvements:
-<div className="w-[64rem] max-w-full rounded-lg border border-solid p-5"> +<div className="w-full max-w-4xl rounded-lg border border-solid p-5">Additionally, if this bordered container style is used elsewhere, consider extracting it to a shared component or utility class for consistency and maintainability.
apps/frontend-manage/src/components/courses/groupActivity/GroupActivityGradingStack.tsx (3)
219-236: Consider adding ARIA labels for better accessibility.The element mapping and StudentElement implementation look good. Consider adding aria-labels to improve accessibility for screen readers.
- <div key={element.id} className="flex flex-col"> + <div + key={element.id} + className="flex flex-col" + aria-label={`Student response for ${element.elementData.name}`} + >
271-296: Consider extracting score calculation logic.The score input implementation is good, but the points calculation could be extracted into a utility function for better maintainability.
+ const calculateMaxPoints = (multiplier: number = 1) => multiplier * pointsPerInstance; <FormikNumberField // ...other props max={calculateMaxPoints(element.options?.pointsMultiplier)} /> <div className="min-w-max"> {t('manage.groupActivity.nPoints', { - number: (element.options?.pointsMultiplier ?? 1) * pointsPerInstance, + number: calculateMaxPoints(element.options?.pointsMultiplier), })} </div>
299-311: Enhance score parsing robustness.The total points calculation could be made more robust by using a dedicated parsing function.
+ const parseScore = (score: string | undefined): number => { + if (!score) return 0; + const parsed = parseFloat(score); + return isNaN(parsed) ? 0 : parsed; + } achieved: values.grading.reduce( - (acc: number, result) => { - return acc + (String(result.score) === '' ? 0 : parseFloat(String(result.score ?? 0))) - }, + (acc: number, result) => acc + parseScore(String(result.score)), 0 ),apps/frontend-manage/src/components/questions/manipulation/StudentElementPreview.tsx (1)
19-19: Enhance type safety for answer collection entriesConsider creating a dedicated type for answer collection entries to improve type safety and reusability.
+interface AnswerCollectionEntry { + id: number + value: string +} + interface StudentElementPreviewProps { values: ElementFormTypes elementDataTypename: ElementData['__typename'] - answerCollectionEntries?: { id: number; value: string }[] + answerCollectionEntries?: AnswerCollectionEntry[] }Also applies to: 108-117
apps/frontend-manage/src/components/questions/manipulation/ElementEditModal.tsx (1)
70-72: Enhance type safety and initial state managementConsider creating a shared type for answer collection entries and using a constant for the initial state.
+interface AnswerCollectionEntry { + id: number + value: string +} + +const INITIAL_ANSWER_COLLECTION_ENTRIES: AnswerCollectionEntry[] = [] + -const [answerCollectionEntries, setAnswerCollectionEntries] = useState< - { id: number; value: string }[] ->([]) +const [answerCollectionEntries, setAnswerCollectionEntries] = useState<AnswerCollectionEntry[]>( + INITIAL_ANSWER_COLLECTION_ENTRIES +)apps/frontend-manage/src/components/questions/manipulation/useValidationSchema.ts (1)
372-388: LGTM! Consider adding a comment explaining the max constraint.The validation logic for
numberOfInputsis correct. The maximum constraint ofnumberOfAnswerOptions - 1makes sense as it ensures there's always at least one option that can be selected for each input field.Consider adding a comment explaining why the maximum constraint is set to
numberOfAnswerOptions - 1:numberOfInputs: yup .number() .required(t('manage.formErrors.SEnumberOfInputsRequired')) .min(1, t('manage.formErrors.SEnumberOfInputsMin')) + // Maximum is set to numberOfAnswerOptions - 1 to ensure there's always at least + // one option that can be selected for each input field .max( numberOfAnswerOptions ? numberOfAnswerOptions - 1 : 100, t('manage.formErrors.SEnumberOfInputsMax') ),packages/shared-components/src/questions/SELECTIONAnswerOptions.tsx (2)
32-38: Consider memoizing the translation messageThe translation message is recreated on every render. Consider memoizing it with useMemo for better performance in cases where the component re-renders frequently.
+ const translatedMessage = useMemo( + () => + t.rich('shared.questions.seSelectNCorrectOptions', { + number: options.numberOfInputs, + b: (text) => <b>{text}</b>, + }), + [t, options.numberOfInputs] + ) return ( <div> <div className="mb-3"> - {t.rich('shared.questions.seSelectNCorrectOptions', { - number: options.numberOfInputs, - b: (text) => <b>{text}</b>, - })} + {translatedMessage} </div>
54-63: Consider memoizing items array to prevent unnecessary re-rendersThe items array is recreated on every render. Since this is used in a mapping operation, memoizing it could prevent unnecessary re-renders of child SelectField components.
+ const selectItems = useMemo( + () => + options.answerCollection?.entries?.map((entry) => ({ + label: entry.value, + value: String(entry.id), + data: { cy: `select-answer-${entry.value}` }, + disabled: selectedValues.includes(entry.id) && selectedValue !== entry.id, + })) ?? [], + [options.answerCollection?.entries, selectedValues, selectedValue] + ) <SelectField required value={selectedValue ? String(selectedValue) : undefined} onChange={(newValue) => { onChange({ ...responses, [inputIndex]: parseInt(newValue) }) }} - items={ - options.answerCollection?.entries?.map((entry) => ({ - label: entry.value, - value: String(entry.id), - data: { cy: `select-answer-${entry.value}` }, - disabled: - selectedValues.includes(entry.id) && - selectedValue !== entry.id, - })) ?? [] - } + items={selectItems}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/shared-components/dist/utilities.cssis excluded by!**/dist/**
📒 Files selected for processing (15)
apps/frontend-manage/src/components/courses/groupActivity/GroupActivityGradingStack.tsx(2 hunks)apps/frontend-manage/src/components/questions/manipulation/ElementEditModal.tsx(3 hunks)apps/frontend-manage/src/components/questions/manipulation/StudentElementPreview.tsx(3 hunks)apps/frontend-manage/src/components/questions/manipulation/options/SelectionOptions.tsx(2 hunks)apps/frontend-manage/src/components/questions/manipulation/useValidationSchema.ts(2 hunks)apps/frontend-manage/src/pages/questions/[id].tsx(2 hunks)apps/frontend-pwa/src/components/groupActivity/GroupActivityStack.tsx(1 hunks)apps/frontend-pwa/src/pages/session/[id].tsx(1 hunks)packages/grading/src/index.ts(1 hunks)packages/grading/test/index.test.ts(2 hunks)packages/i18n/messages/de.ts(3 hunks)packages/i18n/messages/en.ts(3 hunks)packages/shared-components/src/SelectionQuestion.tsx(3 hunks)packages/shared-components/src/StudentElement.tsx(3 hunks)packages/shared-components/src/questions/SELECTIONAnswerOptions.tsx(3 hunks)
✅ Files skipped from review due to trivial changes (2)
- apps/frontend-pwa/src/components/groupActivity/GroupActivityStack.tsx
- apps/frontend-pwa/src/pages/session/[id].tsx
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: build
- GitHub Check: check
- GitHub Check: format
- GitHub Check: build
- GitHub Check: Analyze (javascript)
- GitHub Check: cypress-run
- GitHub Check: SonarCloud
- GitHub Check: test
🔇 Additional comments (15)
apps/frontend-manage/src/pages/questions/[id].tsx (1)
11-11: LGTM!The import statement is correctly structured and the
H2component is appropriately used in the code.apps/frontend-manage/src/components/courses/groupActivity/GroupActivityGradingStack.tsx (4)
211-218: LGTM! Good use of fragments and user feedback.The component structure is clean and the warning notification for completed grading provides clear feedback to users.
237-270: LGTM! Well-optimized feedback form implementation.Good use of FastField for performance optimization and comprehensive error handling. The ContentInput implementation with proper validation is solid.
312-377: LGTM! Well-structured feedback and submission implementation.The general feedback section is well implemented with proper visual feedback, form handling, and state management.
Line range hint
378-398: LGTM! Proper toast notification implementation.The toast notifications are well implemented with appropriate durations and dismissal handling.
apps/frontend-manage/src/components/questions/manipulation/ElementEditModal.tsx (1)
76-78: LGTM!The validation schema update correctly uses the number of answer options.
apps/frontend-manage/src/components/questions/manipulation/useValidationSchema.ts (1)
413-426: LGTM! Parameter propagation is correct.The changes correctly propagate the
numberOfAnswerOptionsparameter touseOptionsSchemaSelection, maintaining the validation chain.packages/grading/test/index.test.ts (1)
255-337: LGTM! Comprehensive test coverage for selection question grading.The test suite thoroughly covers various scenarios for grading selection questions:
- Edge cases (null, empty array, undefined)
- All correct answers
- Partial correct answers
- Single correct answer
- No correct answers
The test data and expected results are well-chosen and logically sound.
packages/i18n/messages/en.ts (1)
15-16: LGTM! Clear and consistent localization strings.The added strings for selection questions are clear and well-synchronized between English and German translations.
Also applies to: 1379-1380
packages/i18n/messages/de.ts (1)
15-16: LGTM! German translations match English strings.The German translations accurately convey the same meaning as their English counterparts.
Also applies to: 1393-1394
packages/shared-components/src/questions/SELECTIONAnswerOptions.tsx (1)
39-44: LGTM! Clean implementation of conditional grid layoutThe use of twMerge for conditional class application is a good practice, and the grid layout responsively adapts based on the preview prop.
packages/shared-components/src/StudentElement.tsx (2)
69-69: LGTM! Clean implementation of optional preview propThe preview prop is properly typed as optional with a sensible default value.
Also applies to: 100-100
335-335: LGTM! Correct prop forwardingThe preview prop is correctly passed only to the SelectionQuestion component where it's needed.
packages/shared-components/src/SelectionQuestion.tsx (2)
22-22: LGTM! Proper type definition and prop usageThe preview prop is correctly typed as required and properly destructured from props.
Also applies to: 35-35
71-71: LGTM! Correct prop forwarding to child componentThe preview prop is properly passed to the SELECTIONAnswerOptions component.
apps/frontend-manage/src/components/questions/manipulation/options/SelectionOptions.tsx
Show resolved
Hide resolved
klicker-uzh
|
||||||||||||||||||||||||||||
| Project |
klicker-uzh
|
| Branch Review |
selection-questions-grading
|
| Run status |
|
| Run duration | 14m 54s |
| Commit |
|
| Committer | Julius Schlapbach |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
202
|
| View all changes introduced in this branch ↗︎ | |
|
Summary by CodeRabbit
New Features
Improvements
Localization
Bug Fixes