fix(apps/frontend-manage): ensure that course dates can be modified despite missing activities #4336
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 hooks within the frontend application. The primary focus is on improving the handling of default values using the nullish coalescing operator ( Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (15)
✅ Files skipped from review due to trivial changes (1)
🔇 Additional comments (19)
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 |
FixCourseManipulationModal
|
| Run status |
|
| Run duration | 10m 43s |
| Commit |
|
| Committer | Julius Schlapbach |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
133
|
| View all changes introduced in this branch ↗︎ | |
|
klicker-uzh
|
||||||||||||||||||||||||||||
| Project |
klicker-uzh
|
| Branch Review |
v3
|
| Run status |
|
| Run duration | 10m 37s |
| Commit |
|
| Committer | Julius Schlapbach |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
|
Failures
|
0
|
|
Flaky
|
0
|
|
Pending
|
0
|
|
Skipped
|
0
|
|
Passing
|
133
|
| 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.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (12)
apps/frontend-manage/src/components/evaluation/hooks/useEvaluationHistogramData.tsx (1)
30-34: Consider adding tests for edge cases.The min/max calculations handle several cases (configured values, computed values, fallbacks). To ensure robustness, consider adding tests for edge cases:
- Responses containing zero values
- Empty response sets
- Single response sets
- Responses with all same values
Would you like me to help generate comprehensive test cases for these scenarios?
apps/frontend-manage/src/components/evaluation/elements/NumericalSidebar.tsx (1)
Line range hint
27-54: Consider removing commented-out statistics code.The TODO comment and large block of commented code could be removed to improve code maintainability. If this feature is planned for the future, it would be better tracked in a separate issue.
Would you like me to help create a GitHub issue to track the reintroduction of statistics functionality?
apps/frontend-manage/src/components/questions/manipulation/helpers.ts (2)
119-120: LGTM! Consider DRY refactor for validation logic.The explicit null and undefined checks improve the robustness of the validation logic. However, since this validation pattern is repeated for both min and max, consider extracting it into a helper function.
+const isEmptyRestriction = (value: any) => + value === null || + typeof value === 'undefined' || + value === ''; restrictions: { min: !values.options.restrictions || - values.options.restrictions.min === null || - typeof values.options.restrictions.min === 'undefined' || - values.options.restrictions.min === '' + isEmptyRestriction(values.options.restrictions.min) ? undefined : parseFloat(String(values.options.restrictions.min)), max: !values.options.restrictions || - values.options.restrictions.max === null || - typeof values.options.restrictions.max === 'undefined' || - values.options.restrictions.max === '' + isEmptyRestriction(values.options.restrictions.max) ? undefined : parseFloat(String(values.options.restrictions.max)),Also applies to: 126-127
Line range hint
177-182: Fix duplicate condition in maxLength validation.There's a duplicated condition in the maxLength validation. Additionally, consider using the same explicit validation pattern as used in numerical restrictions for consistency.
restrictions: { maxLength: - !values.options.restrictions?.maxLength || - !values.options.restrictions?.maxLength || + !values.options.restrictions?.maxLength || + values.options.restrictions.maxLength === null || + typeof values.options.restrictions.maxLength === 'undefined' || values.options.restrictions.maxLength === '' ? undefined : parseInt(String(values.options.restrictions.maxLength)), },packages/grading/src/index.ts (2)
179-179: Consider adding input validation and bounds checking.While the change to
??operator is good, this function could benefit from additional safeguards:
- Input validation for negative values
- Bounds checking for the final result
- Documentation of expected value ranges
Consider adding validation:
export function computeAwardedPoints({ firstResponseReceivedAt, responseTimestamp, maxBonus, timeToZeroBonus, getsMaxPoints, defaultPoints, defaultCorrectPoints, pointsPercentage, pointsMultiplier, }: ComputeAwardedPointsArgs): number { + // Validate inputs + if (maxBonus < 0) throw new Error('maxBonus must be non-negative') + if (defaultPoints !== undefined && defaultPoints < 0) + throw new Error('defaultPoints must be non-negative') + if (defaultCorrectPoints !== undefined && defaultCorrectPoints < 0) + throw new Error('defaultCorrectPoints must be non-negative') + const slope = maxBonus / (timeToZeroBonus ?? 20) let awardedPoints = 0 // ... existing code ... awardedPoints += defaultPoints ?? 0 - return Math.round(awardedPoints) + // Ensure final result is non-negative + return Math.max(0, Math.round(awardedPoints)) }
Line range hint
165-179: Consider adding unit tests for edge cases.The changes to default value handling are good, but they highlight the need for comprehensive testing of edge cases, particularly:
- Zero point values
- Null/undefined inputs
- Boundary conditions for time-based calculations
- Combinations of different point types (default, correct, bonus)
Consider adding test cases that verify:
- Zero points are preserved and calculated correctly
- Time-based bonus calculations handle edge cases
- Point multipliers work correctly with zero and null values
- Rounding behavior maintains consistency across calculations
apps/frontend-pwa/src/components/practiceQuiz/ElementSummary.tsx (1)
Line range hint
6-6: Avoid usinganytype as it bypasses TypeScript's type checking.Using
anytype reduces type safety and could lead to runtime errors. Consider using a more specific type or interface that accurately represents the expected stack structure.Could you share the original stack type? This would help in suggesting a proper type definition that maintains type safety while addressing your needs.
packages/shared-components/src/Histogram.tsx (3)
85-85: Consider extracting domain padding values to constants.The magic numbers
-10and+10used for padding the domain could be extracted into named constants to improve maintainability and make the padding configurable.+const DOMAIN_PADDING = 10 const min: number = questionData.options.restrictions && typeof questionData.options.restrictions['min'] === 'number' ? questionData.options.restrictions['min'] - : (minBy(mappedData, 'value')?.value ?? 0) - 10 + : (minBy(mappedData, 'value')?.value ?? 0) - DOMAIN_PADDING const max: number = questionData.options.restrictions && typeof questionData.options.restrictions['max'] === 'number' ? questionData.options.restrictions['max'] - : (maxBy(mappedData, 'value')?.value ?? 0) + 10 + : (maxBy(mappedData, 'value')?.value ?? 0) + DOMAIN_PADDINGAlso applies to: 90-90
Line range hint
91-114: Consider memoizing bin calculations for better performance.The bin calculations in this section are performed on every render. Consider memoizing the
dataArraycalculations usinguseMemoto optimize performance, especially for large datasets.-let dataArray = Array.from({ length: binCount }, (_, i) => ({ +const dataArray = useMemo(() => { + const initial = Array.from({ length: binCount }, (_, i) => ({ value: min + (max - min) * (i / binCount) + (max - min) / (2 * binCount), -})) + })); -dataArray = dataArray.map((bin) => { + return initial.map((bin) => { const binWidth = - dataArray.length > 1 ? dataArray[1]!.value - dataArray[0]!.value : 1 + initial.length > 1 ? initial[1]!.value - initial[0]!.value : 1 const count = sumBy( mappedData.filter((result) => { return ( result.value >= bin.value - binWidth / 2 && (bin.value + binWidth / 2 === max ? result.value <= max : result.value < bin.value + binWidth / 2) ) }), 'count' ) return { value: round(bin.value, 2), count, label: `${round(bin.value - binWidth / 2, 1)} - ${round( bin.value + binWidth / 2, 1 )}`, } -}) +}); +}, [binCount, min, max, mappedData]);
Line range hint
249-277: Address the TODO comment regarding exact solutions support.There's a TODO comment indicating pending migration work for exact solutions support on numerical questions. This technical debt should be tracked and addressed.
Would you like me to help create a GitHub issue to track this migration work? I can provide a detailed description of the required changes and implementation approach.
apps/frontend-manage/src/components/sessions/cockpit/SessionTimeline.tsx (1)
Line range hint
91-143: Consider optimizing state updates to prevent potential re-renders.The useEffect hook's dependency on
activeBlockIdwhile also setting it within the effect could lead to unnecessary re-renders. Consider:
- Combining related state updates using a reducer
- Moving the
activeBlockIdcalculation outside the effectHere's a suggested refactor using useReducer:
interface TimelineState { activeBlockId: number; lastActiveBlockId: number; buttonState: 'firstBlock' | 'blockActive' | 'nextBlock' | 'endSession'; inCooldown: boolean; } type TimelineAction = | { type: 'UPDATE_BLOCKS'; blocks: SessionTimelineBlock[] } | { type: 'SET_COOLDOWN'; value: boolean }; function timelineReducer(state: TimelineState, action: TimelineAction): TimelineState { switch (action.type) { case 'UPDATE_BLOCKS': { const activeBlockId = action.blocks.find((block) => block.status === 'ACTIVE')?.id ?? -1; const lastActiveBlockId = calculateLastActiveBlockId(action.blocks); return { ...state, activeBlockId, lastActiveBlockId, buttonState: calculateButtonState(activeBlockId, lastActiveBlockId, action.blocks), inCooldown: activeBlockId === -1 ? false : state.inCooldown, }; } case 'SET_COOLDOWN': return { ...state, inCooldown: action.value }; } }packages/graphql/src/services/practiceQuizzes.ts (1)
Line range hint
1750-1754: Consider using optional chaining with nullish coalescing.The score assignment could be simplified using optional chaining with nullish coalescing.
Consider this more concise version:
- const score = evaluation?.score ?? 0 + let xp: number | null + if ( + elementData.type === ElementType.CONTENT || + elementData.type === ElementType.FLASHCARD + ) { + xp = 0 + } else { + xp = elementData.options.hasSampleSolution ? evaluation?.xp ?? 0 : 0 + }
🛑 Comments failed to post (6)
apps/frontend-manage/src/lib/hooks/useEarliestLatestCourseDates.ts (2)
58-65: 🛠️ Refactor suggestion
Consider extracting the date calculation pattern.
The pattern for safe date calculations is repeated three times. Consider extracting it into a utility function for better maintainability.
const getSafeDate = ( dates: number[], operation: 'min' | 'max' ): string | undefined => { if (dates.length === 0) return undefined const fn = operation === 'min' ? Math.min : Math.max return dayjs(fn.apply(null, dates)).toString() } // Usage in return object: return { earliestGroupDeadline: getSafeDate(groupActivityStartDates, 'min'), earliestStartDate: getSafeDate(startDates, 'min'), latestEndDate: getSafeDate(endDates, 'max'), }This refactor would:
- Reduce code duplication
- Make the pattern more maintainable
- Centralize the date calculation logic
40-46: 🛠️ Refactor suggestion
Simplify the practice quiz filtering logic.
The null/undefined check can be simplified while maintaining type safety.
- ...(practiceQuizzes - ?.filter( - (pq) => - pq.availableFrom !== null && - typeof pq.availableFrom !== 'undefined' - ) - .map((pq) => Date.parse(pq.availableFrom)) ?? []), + ...(practiceQuizzes?.filter((pq) => pq.availableFrom != null) + .map((pq) => Date.parse(pq.availableFrom)) ?? []),The
!= nullcheck in JavaScript handles bothnullandundefinedcases, making the explicit checks redundant.📝 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....(practiceQuizzes?.filter((pq) => pq.availableFrom != null) .map((pq) => Date.parse(pq.availableFrom)) ?? []),apps/frontend-manage/src/components/sessions/creation/liveQuiz/AdvancedLiveQuizSettings.tsx (3)
89-96: 🛠️ Refactor suggestion
Consider refactoring repeated number parsing logic.
The component has multiple instances of number parsing with similar validation needs. This could be simplified and made more maintainable.
Consider these improvements:
- Create a utility function for safe number parsing:
const safeParseInt = (value: string, fallback = 0): number => { const parsed = parseInt(value, 10); return Number.isNaN(parsed) ? fallback : parsed; };
- Memoize the chart data to prevent unnecessary recalculations:
const chartData = useMemo(() => [ { time: 0, points: SUMMED_CORRECT_PTS + safeParseInt(maxBonusValue), }, { time: safeParseInt(timeToZeroValue), points: SUMMED_CORRECT_PTS, }, { time: 2 * safeParseInt(timeToZeroValue), points: SUMMED_CORRECT_PTS, }, ], [maxBonusValue, timeToZeroValue]);
89-89:
⚠️ Potential issueAdd validation for non-numeric input.
The current implementation doesn't handle invalid numeric inputs properly. If
maxBonusValueis not a valid number,parseIntwill returnNaNwhich won't be caught by the nullish coalescing operator.Consider using a more robust number parsing approach:
-SUMMED_CORRECT_PTS + (parseInt(maxBonusValue, 10) ?? 0), +SUMMED_CORRECT_PTS + (Number.isNaN(parseInt(maxBonusValue, 10)) ? 0 : parseInt(maxBonusValue, 10)),📝 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.SUMMED_CORRECT_PTS + (Number.isNaN(parseInt(maxBonusValue, 10)) ? 0 : parseInt(maxBonusValue, 10)),
92-96:
⚠️ Potential issueAdd validation for non-numeric input and ensure consistent null safety.
Similar to the bonus points calculation, the time calculations don't handle invalid numeric inputs properly. Additionally, the XAxis domain calculation at line 111 is missing the same null safety.
Apply the same robust number parsing approach:
-time: parseInt(timeToZeroValue, 10) ?? 0, +time: Number.isNaN(parseInt(timeToZeroValue, 10)) ? 0 : parseInt(timeToZeroValue, 10), -time: 2 * (parseInt(timeToZeroValue, 10) ?? 0), +time: 2 * (Number.isNaN(parseInt(timeToZeroValue, 10)) ? 0 : parseInt(timeToZeroValue, 10)),Also update the XAxis domain:
-domain={[0, 2 * parseInt(timeToZeroValue)]} +domain={[0, 2 * (Number.isNaN(parseInt(timeToZeroValue, 10)) ? 0 : parseInt(timeToZeroValue, 10))]}Committable suggestion was skipped due to low confidence.
packages/prisma/src/data/seedTEST.ts (1)
980-980: 🛠️ Refactor suggestion
Refactor duplicated
maxPointscalculation into a helper functionThe calculation of
maxPointsin lines 980 and 1024 is identical. To adhere to the DRY (Don't Repeat Yourself) principle and improve maintainability, consider extracting this logic into a helper function.Define a helper function:
function calculateMaxPoints(element) { return (element.options?.pointsMultiplier ?? 1) * 25; // default: 25 points }Then, update the lines to use the helper function:
- const maxPoints = (element.options.pointsMultiplier ?? 1) * 25 // default: 25 points + const maxPoints = calculateMaxPoints(element);Also applies to: 1024-1024
Summary by CodeRabbit
Release Notes
Bug Fixes
nullandundefinedvalues across various components.New Features
pointsMultiplierand date calculations in course settings.Documentation
These updates enhance the overall stability and user experience of the grading and evaluation features.