Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: migrate numerical question evaluation on asynchronous activities to new histogram #4340

Merged
merged 6 commits into from
Oct 29, 2024

Conversation

sjschlapbach
Copy link
Member

@sjschlapbach sjschlapbach commented Oct 29, 2024

Screenshot 2024-10-29 at 16 13 29

Summary by CodeRabbit

  • New Features

    • Enhanced chart components by utilizing a shared components library for improved modularity.
    • Updated data handling in several components to accommodate new data structures for better performance.
    • Introduced new test cases for creating various types of questions in the Cypress test suite.
  • Bug Fixes

    • Resolved import path issues for various components to ensure proper functionality.
    • Improved validation logic for min, max, and maxLength values in the StudentElementPreview component.
  • Documentation

    • Updated TypeScript type imports for clarity and improved type-checking.
  • Chores

    • Modified package.json files for dependency management and ensured consistency across shared components.

Copy link

coderabbitai bot commented Oct 29, 2024

📝 Walkthrough
📝 Walkthrough

Walkthrough

This pull request introduces modifications across several files, primarily focusing on updates to dependency management in package.json files for both the frontend-manage and shared-components projects. Dependencies have been removed and added, with specific version changes. Additionally, several components have shifted to utilize a shared components library, affecting import paths and data handling for charts and tables. The tsconfig.json file has been updated to include new TypeScript files, enhancing the project's type-checking capabilities.

Changes

File Change Summary
apps/frontend-manage/package.json Removed dependencies: @tanstack/react-table, react-csv-downloader, react-tagcloud; added react-tagcloud as a devDependency.
apps/frontend-manage/src/components/courses/IndividualLeaderboard.tsx Updated import path for DataTable.
apps/frontend-manage/src/components/evaluation/ElementChart.tsx Updated imports for chart components from local to shared library. Modified props for ElementHistogram.
packages/shared-components/package.json Added new devDependencies: @tanstack/react-table, @types/react-tagcloud, react-csv-downloader, react-tagcloud.
packages/shared-components/src/DataTable.tsx Updated type imports for ColumnDef and SortingState; modified React import.
packages/shared-components/src/charts/ElementBarChart.tsx Changed import for ElementInstanceEvaluation to type; redefined textSize property in props interface.
packages/shared-components/src/charts/ElementHistogram.tsx Updated props interface to include new data structure; modified internal logic to accommodate changes.
packages/shared-components/src/charts/ElementTableChart.tsx Updated import paths and changed import for ElementInstanceEvaluation to type.
packages/shared-components/src/charts/ElementWordcloud.tsx Changed textSize property type in props interface.
packages/shared-components/src/evaluation/NREvaluation.tsx Updated imports and modified data handling for evaluation answers.
apps/frontend-manage/tsconfig.json Expanded include array to add new TypeScript files.
packages/shared-components/src/hooks/useEvaluationBarChartData.tsx Changed import for ElementInstanceEvaluation to type.
packages/shared-components/src/hooks/useEvaluationHistogramData.tsx Updated interface and function parameters to reflect new data structure.
packages/shared-components/src/hooks/useEvaluationTableColumns.tsx Modified import for useMemo to consolidate React imports.
packages/shared-components/src/hooks/useEvaluationTableData.tsx Changed imports for ElementInstanceEvaluation and EvaluationTableRowType to type.
apps/frontend-manage/src/components/questions/manipulation/StudentElementPreview.tsx Improved validation logic for min, max, and maxLength properties in restrictions.
cypress/cypress/e2e/D-questions-workflow.cy.ts Activated previously commented-out test cases for various question types in Cypress tests.

Suggested reviewers

  • rschlaefli

📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between addb65e and 70c9cb9.

📒 Files selected for processing (1)
  • apps/frontend-manage/src/components/questions/manipulation/StudentElementPreview.tsx (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • apps/frontend-manage/src/components/questions/manipulation/StudentElementPreview.tsx

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.

Actionable comments posted: 9

🧹 Outside diff range and nitpick comments (8)
packages/shared-components/src/evaluation/NREvaluation.tsx (1)

Line range hint 31-42: Improve reference value parsing.

The reference value parsing could be more robust. Consider handling potential NaN values and moving the parsing logic outside the JSX for better readability.

+ const parsedReference = reference ? parseFloat(reference) : undefined
+ const isValidReference = parsedReference !== undefined && !Number.isNaN(parsedReference)

  <ElementHistogram
    type={ElementType.Numerical}
    responses={responses}
    solutionRanges={options.solutionRanges ?? undefined}
    minValue={options.restrictions?.min}
    maxValue={options.restrictions?.max}
    showSolution={{ general: true }}
    textSize="md"
    className={{ root: 'h-40' }}
-   reference={reference ? parseFloat(reference) : undefined}
+   reference={isValidReference ? parsedReference : undefined}
    hideBins
    basic
  />
packages/shared-components/src/charts/ElementTableChart.tsx (1)

9-10: Architectural concern: Shared component depends on app-specific code.

The hooks useEvaluationTableColumns and useEvaluationTableData are imported from the frontend-manage app, which creates tight coupling between packages. Shared components should be independent and not rely on app-specific code.

Consider moving these hooks to the shared-components package or creating a separate package for shared hooks. This would:

  • Maintain package independence
  • Improve code reusability
  • Follow better architectural practices

Example restructure:

// Move to @klicker-uzh/shared-components/src/hooks/evaluation/useEvaluationTableColumns.ts
// Move to @klicker-uzh/shared-components/src/hooks/evaluation/useEvaluationTableData.ts
apps/frontend-manage/src/components/courses/IndividualLeaderboard.tsx (1)

Line range hint 63-63: Consider using a more robust filename sanitization method.

The current CSV filename generation using simple space replacement might not handle all special characters properly. Consider using a dedicated function to sanitize the filename.

-csvFilename={`${courseName.replace(' ', '-')}_leaderboard`}
+csvFilename={`${sanitizeFilename(courseName)}_leaderboard`}

Add this helper function:

function sanitizeFilename(filename: string): string {
  return filename
    .replace(/[^a-z0-9]/gi, '-')
    .replace(/-+/g, '-')
    .toLowerCase();
}
apps/frontend-manage/src/components/evaluation/ElementChart.tsx (2)

2-5: LGTM! Good architectural improvement.

Moving chart components to a shared library enhances modularity and promotes code reuse across the application.


47-51: Consider adding type safety for numerical evaluation props.

The props update nicely separates concerns and makes the data flow more explicit. To enhance type safety, consider creating a dedicated type for numerical evaluation props.

interface NumericalEvaluationProps {
  type: string;
  responses: Array<{ value: number; count: number }>;
  solutionRanges: Array<{ min: number; max: number }>;
  minValue: number;
  maxValue: number;
}
packages/shared-components/src/charts/ElementBarChart.tsx (1)

11-11: Consider removing unnecessary React import

With modern React and the new JSX transform, explicit React imports are not required unless you're using React namespace directly (e.g., React.useState).

-import React from 'react'
packages/shared-components/src/charts/ElementHistogram.tsx (2)

18-18: Consider moving useEvaluationHistogramData to shared-components for better modularity

The useEvaluationHistogramData hook is being imported from a relative path that reaches into another package (frontend-manage). To promote modularity and reusability, consider moving this hook into the shared-components package.


Line range hint 118-126: Handle reference value of 0 correctly

The condition {reference && ( will skip rendering the ReferenceLine if reference is 0, because 0 is falsy in JavaScript/TypeScript. If reference can be 0, you should explicitly check for reference !== undefined to ensure the ReferenceLine is rendered correctly.

Apply this diff:

-           {reference && (
+           {reference !== undefined && (
🛑 Comments failed to post (9)
packages/shared-components/src/evaluation/NREvaluation.tsx (2)

23-26: ⚠️ Potential issue

Add error handling for numerical value parsing.

The current implementation assumes all values can be successfully parsed to float. Consider adding error handling to gracefully handle invalid numerical values.

   const responses = Object.entries(answers).map(([_, answer]) => ({
-    value: parseFloat(answer.value),
+    value: Number.isNaN(parseFloat(answer.value)) 
+      ? 0 // or handle invalid values differently
+      : parseFloat(answer.value),
     count: answer.count,
   }))
📝 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.

  const responses = Object.entries(answers).map(([_, answer]) => ({
    value: Number.isNaN(parseFloat(answer.value)) 
      ? 0 // or handle invalid values differently
      : parseFloat(answer.value),
    count: answer.count,
  }))

19-22: 💡 Codebase verification

The 'correct' field in answers type is not needed and can be safely removed

The correct field in the answers type is not being used in the current implementation:

  • The responses array is mapped to only use value and count properties
  • The ElementHistogram component accepts responses with only value and count properties
  • Correctness is handled separately through solutionRanges prop in the visualization
🔗 Analysis chain

Verify if the 'correct' field is needed in the answers type.

The type definition includes a 'correct' boolean field, but this field isn't used in the data transformation. Consider removing it if it's not needed for the current implementation.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for usages of the 'correct' field from answers
rg -g '*.{ts,tsx}' "answers.*\.correct" 

Length of output: 41


Script:

#!/bin/bash
# Let's try a broader search for 'correct' field usage in evaluation-related files
rg -g '*.{ts,tsx}' "correct.*boolean" -A 2 packages/shared-components/src/evaluation/

# Also search for any references to 'correct' in the specific file
cat packages/shared-components/src/evaluation/NREvaluation.tsx

# Look for interfaces or types that might define this structure
ast-grep --pattern 'interface $_ {
  $$$
  correct: boolean
  $$$
}'

ast-grep --pattern 'type $_ = {
  $$$
  correct: boolean
  $$$
}'

Length of output: 3631


Script:

#!/bin/bash
# Let's check how the 'correct' field is used in the ElementHistogram component
cat packages/shared-components/src/charts/ElementHistogram.tsx

# Also check if the correct field is used in the transformation of responses
rg -g '*.{ts,tsx}' "responses.*map.*correct" -A 2

Length of output: 7704

packages/shared-components/src/charts/ElementWordcloud.tsx (1)

13-16: 💡 Codebase verification

Breaking change in ElementWordcloud requires type adjustments

The verification revealed incompatible type definitions:

  • ElementWordcloud expects textSize: { min: number, max: number }
  • ElementChart passes textSize={{ min: textSize.min, max: textSize.max }}, but its TextSizeType is defined as { size: string, text: string, prose: string, textLg: string, textXl: string }

This will cause type errors as the properties min and max don't exist on TextSizeType. The following changes are needed:

  • Update TextSizeType in apps/frontend-manage/src/components/sessions/evaluation/constants.ts to include min and max number properties
  • Or create a separate type for the wordcloud's text size props
🔗 Analysis chain

Verify the impact of the breaking change in textSize prop.

The new type structure for textSize is more specific and type-safe. However, this is a breaking change that requires all consumers of this component to update their prop structure from Record<string, number> to the new { min: number; max: number } format.

Let's verify the impact:


Let me generate additional scripts to verify the type definition and its usage:

The previous results show that ElementChart is using the component with the new structure correctly (textSize={{ min: textSize.min, max: textSize.max }}), but we need to verify the type definition to ensure full compatibility.


Let me check one more thing to verify the TextSizeType definition:

This will help us understand how TextSizeType is defined and used in the consuming component.


Let's check the TextSizeType definition in the constants file:

This will help us confirm if the type definition in the consuming component is compatible with the breaking change.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Find all usages of ElementWordcloud to ensure they're updated
# to use the new textSize prop structure

# Search for ElementWordcloud imports and usages
rg -l "ElementWordcloud" | xargs rg -A 5 "textSize"

# Search for any potential test files that might need updates
fd -e test.tsx -e test.ts | xargs rg -A 5 "ElementWordcloud"

Length of output: 4748


Script:

#!/bin/bash
# Check the type definition of TextSizeType in ElementChart
rg -A 5 "type TextSizeType" apps/frontend-manage/src/components/evaluation/ElementChart.tsx

# Check if there are any other files importing and using ElementWordcloud
rg "import.*ElementWordcloud" 

Length of output: 279


Script:

#!/bin/bash
# Get the full content of ElementChart.tsx to see the TextSizeType definition
rg -B 10 -A 20 "interface ElementChartProps" apps/frontend-manage/src/components/evaluation/ElementChart.tsx

Length of output: 1248


Script:

#!/bin/bash
# Check the TextSizeType definition in the constants file
rg -A 5 "TextSizeType" apps/frontend-manage/src/components/sessions/evaluation/constants.ts

Length of output: 203

apps/frontend-manage/src/components/evaluation/hooks/useEvaluationHistogramData.tsx (2)

21-22: ⚠️ Potential issue

Add validation for empty responses array

The current implementation doesn't handle the case where the responses array is empty. Consider adding a check to return early or provide default values.

  if (type !== ElementType.Numerical) {
    return null
  }
+ if (!responses.length) {
+   return { data: [], domain: { min: 0, max: 0 } }
+ }
📝 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.

    if (type !== ElementType.Numerical) {
      return null
    }
    if (!responses.length) {
      return { data: [], domain: { min: 0, max: 0 } }
    }

25-32: 🛠️ Refactor suggestion

Consider parameterizing the fallback padding values

The hardcoded padding values of ±10 for min/max calculations might not be suitable for all numerical ranges. Consider making these configurable or calculating them based on the data range.

+ interface UseEvaluationHistogramDataProps {
+   type: ElementType
+   responses: { value: number; count: number }[]
+   minValue?: number | null
+   maxValue?: number | null
+   binCount: number
+   fallbackPadding?: number
+ }

  const min: number =
    minValue !== null && typeof minValue === 'number'
      ? minValue
-     : (minBy(responses, 'value')?.value ?? 0) - 10
+     : (minBy(responses, 'value')?.value ?? 0) - (fallbackPadding ?? 10)
  const max: number =
    maxValue !== null && typeof maxValue === 'number'
      ? maxValue
-     : (maxBy(responses, 'value')?.value ?? 0) + 10
+     : (maxBy(responses, 'value')?.value ?? 0) + (fallbackPadding ?? 10)

Committable suggestion was skipped due to low confidence.

packages/shared-components/package.json (2)

19-26: ⚠️ Potential issue

Fix version constraint inconsistency for react-tagcloud.

The version constraints between react-tagcloud (~2.3.3) and @types/react-tagcloud (^2.3.2) are inconsistent. The types package allows minor updates while the main package only allows patch updates. This could lead to type mismatches if the types package updates to a newer minor version.

Apply this diff to align the version constraints:

-    "@types/react-tagcloud": "^2.3.2",
+    "@types/react-tagcloud": "~2.3.2",
📝 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.

    "@types/react-tagcloud": "~2.3.2",
    "autoprefixer": "~10.4.20",
    "eslint": "~8.45.0",
    "lodash": "~4.17.21",
    "npm-run-all": "~4.1.5",
    "postcss-import": "~16.1.0",
    "react-csv-downloader": "~3.1.1",
    "react-tagcloud": "~2.3.3",

13-13: 💡 Codebase verification

Move runtime dependencies to the correct section

Based on the verification results, these packages are indeed used as runtime dependencies in the component source files:

  • @tanstack/react-table is imported and used in DataTable.tsx
  • react-tagcloud is imported as TagCloud in ElementWordcloud.tsx
  • react-csv-downloader is imported in DataTable.tsx

These packages should be moved from devDependencies to dependencies as they are required for the components to function at runtime.

🔗 Analysis chain

Verify dependency configuration.

These newly added packages are currently in devDependencies, but they appear to be runtime dependencies used in the distributed package. If these packages are required for the components to function:

  1. Move them to regular dependencies if they're needed at runtime
  2. Add them to peerDependencies if consumers should provide their own versions

Let's verify the usage of these packages in the codebase:

Also applies to: 19-19, 25-25, 26-26

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check if these packages are used in the source code
# and not just in tests or development tools

# Search for imports/usage of these packages
echo "Checking usage of new dependencies..."
rg -l '@tanstack/react-table|react-tagcloud|react-csv-downloader' --type ts --type tsx

Length of output: 201


Script:

#!/bin/bash
# Let's try a different approach by searching for any file extensions
echo "Finding all possible file extensions first..."
fd . --type f | grep -o '\.[^.]*$' | sort -u

echo -e "\nNow searching for package usage with found extensions..."
rg -l '@tanstack/react-table|react-tagcloud|react-csv-downloader'

echo -e "\nChecking package.json location and content..."
fd package.json
cat packages/shared-components/package.json

Length of output: 4118


Script:

#!/bin/bash
# Let's check the actual usage of these packages in the source files
echo "Checking @tanstack/react-table usage:"
rg -C 3 '@tanstack/react-table' packages/shared-components/src/DataTable.tsx

echo -e "\nChecking react-tagcloud usage:"
rg -C 3 'react-tagcloud' packages/shared-components/src/charts/ElementWordcloud.tsx

echo -e "\nChecking react-csv-downloader usage:"
rg -C 3 'react-csv-downloader' packages/shared-components/

Length of output: 1937

packages/shared-components/src/charts/ElementBarChart.tsx (1)

22-22: ⚠️ Potential issue

Critical: Fix package boundary violation

The shared-components package should not directly depend on the frontend-manage app. This creates tight coupling and potential circular dependencies. The hook should either be:

  1. Moved to the shared-components package if it's truly shared functionality
  2. Passed as a prop if it's specific to the frontend-manage app

Consider refactoring to maintain proper package boundaries:

  1. If the hook is reusable:
-import useEvaluationBarChartData from '../../../../apps/frontend-manage/src/components/evaluation/hooks/useEvaluationBarChartData'
+import useEvaluationBarChartData from '../../hooks/useEvaluationBarChartData'
  1. If the hook is app-specific:
interface ElementBarChartProps {
  instance: ElementInstanceEvaluation
  showSolution: boolean
  textSize: {
    legend: string
    textXl: string
    text3Xl: string
  }
+ getChartData: (params: { instance: ElementInstanceEvaluation }) => ChartData[]
}
packages/shared-components/src/charts/ElementHistogram.tsx (1)

60-60: 🛠️ Refactor suggestion

Remove unnecessary parseInt on numBins

After initializing numBins as a number, the parseInt(numBins) becomes unnecessary. You can pass numBins directly to the useEvaluationHistogramData hook.

Apply this diff:

-   binCount: parseInt(numBins),
+   binCount: numBins,

Committable suggestion was skipped due to low confidence.

Copy link

cypress bot commented Oct 29, 2024

klicker-uzh    Run #3443

Run Properties:  status check passed Passed #3443  •  git commit d47354e738 ℹ️: Merge 70c9cb98109904b557336e37080227894315ef8e into 72fa74954bc4d56bca6349e78f25...
Project klicker-uzh
Branch Review fix-student-histogram
Run status status check passed Passed #3443
Run duration 11m 36s
Commit git commit d47354e738 ℹ️: Merge 70c9cb98109904b557336e37080227894315ef8e into 72fa74954bc4d56bca6349e78f25...
Committer Julius Schlapbach
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 0
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 140
View all changes introduced in this branch ↗︎

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.

Actionable comments posted: 2

🧹 Outside diff range and nitpick comments (4)
apps/frontend-manage/tsconfig.json (1)

30-33: Consider restructuring cross-package dependencies.

Including specific files from another package (shared-components) directly in your TypeScript configuration might indicate a tight coupling between packages. This could make it harder to maintain and version these packages independently.

Consider one of these alternatives:

  1. Move these hooks to the frontend-manage package if they're specific to it
  2. Create proper package exports in shared-components and use them through the package's public API
  3. Set up proper workspace references if using a monorepo setup
packages/shared-components/src/hooks/useEvaluationHistogramData.tsx (1)

5-11: LGTM! Consider adding JSDoc comments.

The interface redesign improves modularity by accepting raw data instead of a specific instance type. This makes the hook more reusable across different contexts.

Add JSDoc comments to document the purpose and constraints of each property:

 interface UseEvaluationHistogramDataProps {
+  /** The type of element being evaluated */
   type: ElementType
+  /** Array of response values and their frequencies */
   responses: { value: number; count: number }[]
+  /** Optional minimum value for the histogram range */
   minValue?: number | null
+  /** Optional maximum value for the histogram range */
   maxValue?: number | null
+  /** Number of bins to divide the data into */
   binCount: number
 }
packages/shared-components/src/charts/ElementHistogram.tsx (2)

Line range hint 133-213: Consider removing commented code and tracking TODO properly.

Large blocks of commented code can lead to maintenance issues. Consider:

  1. Moving this code to a separate file for reference
  2. Creating a proper issue to track the reintroduction of statistics features

Would you like me to help create a GitHub issue to track the reintroduction of statistics visualization for asynchronous elements?


244-254: Add validation for bins input.

While the implementation is clean, consider adding validation to ensure the number of bins is reasonable and doesn't impact performance.

 <NumberField
   precision={0}
   id="histogramBins"
   label={t('manage.evaluation.histogramBins')}
   value={numBins}
+  min={5}
+  max={100}
   onChange={(value) => setNumBins(value)}
 />
🛑 Comments failed to post (2)
packages/shared-components/src/hooks/useEvaluationHistogramData.tsx (1)

25-32: 🛠️ Refactor suggestion

Consider making the padding strategy more configurable.

The current implementation uses hard-coded padding values (±10) when min/max values are not provided. This might not be suitable for all data ranges.

Consider adding padding configuration:

 interface UseEvaluationHistogramDataProps {
   type: ElementType
   responses: { value: number; count: number }[]
   minValue?: number | null
   maxValue?: number | null
   binCount: number
+  padding?: number | { min?: number; max?: number }
 }

Then update the calculations:

-      : (minBy(responses, 'value')?.value ?? 0) - 10
+      : (minBy(responses, 'value')?.value ?? 0) - (padding ?? 10)

Committable suggestion was skipped due to low confidence.

packages/shared-components/src/charts/ElementHistogram.tsx (1)

124-132: 🛠️ Refactor suggestion

Consider enhancing reference line customization.

The reference line implementation is good, but could be more flexible. Consider adding props for:

  • Custom stroke color
  • Custom stroke width
  • Custom label
 {reference && (
   <ReferenceLine
     isFront
     className={textSize}
     key="reference"
-    stroke="red"
+    stroke={referenceLineColor ?? 'red'}
+    strokeWidth={referenceLineWidth ?? 1}
+    label={referenceLineLabel}
     x={reference}
   />
 )}

Committable suggestion was skipped due to low confidence.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 (4)
apps/frontend-manage/src/components/questions/manipulation/StudentElementPreview.tsx (1)

Line range hint 8-14: Consider strengthening type safety for options object.

The current implementation relies heavily on runtime type checking using the 'in' operator. Consider defining explicit interfaces for the options structure to improve type safety and maintainability.

Add a dedicated interface for the restrictions:

interface ElementRestrictions {
  min?: number;
  max?: number;
  maxLength?: number;
}

interface ElementOptions {
  displayMode?: string;
  choices?: Array<any>; // Define proper type for choices
  accuracy?: number;
  unit?: string;
  restrictions?: ElementRestrictions;
}

This would make the type checking more robust and provide better IDE support.

cypress/cypress/e2e/D-questions-workflow.cy.ts (3)

272-311: Add validation tests for numeric question bounds

The numeric question test only verifies the happy path. Consider adding tests for:

  • Invalid minimum/maximum values
  • Minimum greater than maximum
  • Invalid precision values

Add test cases for edge cases:

it('validates numeric question constraints', () => {
  // Test invalid minimum > maximum
  cy.get('[data-cy="set-numerical-minimum"]').clear().type('100')
  cy.get('[data-cy="set-numerical-maximum"]').clear().type('0')
  cy.get('[data-cy="save-new-question"]').should('be.disabled')
  
  // Test invalid precision
  cy.get('[data-cy="set-numerical-accuracy"]').clear().type('-1')
  cy.get('[data-cy="save-new-question"]').should('be.disabled')
})

184-270: Validate KPRIM question constraints

The KPRIM test should verify that exactly 4 options are required. Currently, it only tests the happy path with 4 options.

Add validation tests:

// Test that save is disabled with < 4 options
cy.get('[data-cy="save-new-question"]').should('be.disabled')
cy.get('[data-cy="kp-answer-options"]').should('have.length.lessThan', 4)

// Test that adding more than 4 options is not possible
cy.get('[data-cy="add-new-answer"]').should('be.disabled')

Line range hint 1-390: Enhance test coverage with error scenarios and accessibility tests

The test suite focuses on happy paths but lacks coverage for:

  1. Error handling (network errors, validation errors)
  2. Accessibility testing
  3. Question preview functionality

Add the following test cases:

// Test error handling
it('handles network errors gracefully', () => {
  cy.intercept('POST', '/api/questions', { forceNetworkError: true })
  // Verify error message is displayed
})

// Test accessibility
it('ensures question form is accessible', () => {
  cy.injectAxe()
  cy.checkA11y('[data-cy="question-form"]')
})

// Test preview
it('previews question before saving', () => {
  cy.get('[data-cy="preview-question"]').click()
  // Verify preview matches input
})
🛑 Comments failed to post (3)
apps/frontend-manage/src/components/questions/manipulation/StudentElementPreview.tsx (1)

75-77: 🛠️ Refactor suggestion

Strengthen type checking for min/max restrictions.

The validation logic has been enhanced to be more explicit and robust by adding strict type checking. However, there might be a more concise way to handle this validation.

Consider simplifying the validation using the nullish coalescing operator:

- typeof values.options.restrictions.min !== 'undefined' &&
- values.options.restrictions.min !== null
+ values.options.restrictions.min != null

- typeof values.options.restrictions.max !== 'undefined' &&
- values.options.restrictions.max !== null
+ values.options.restrictions.max != null

This achieves the same strict validation (checking for both undefined and null) but with cleaner syntax. The != operator in JavaScript/TypeScript performs type coercion, making != null check for both null and undefined.

Also applies to: 86-88

cypress/cypress/e2e/D-questions-workflow.cy.ts (2)

94-94: ⚠️ Potential issue

Replace hardcoded waits with proper Cypress commands

Using fixed wait times (cy.wait(500)) is an anti-pattern in Cypress as it makes tests brittle and potentially slower than necessary. Instead, use Cypress's built-in retry-ability and wait for specific elements or states.

Replace the waits with proper assertions:

- cy.wait(500)
+ cy.get('[data-cy="insert-answer-field-1"]').should('exist')

Also applies to: 154-154, 214-214, 218-218, 222-222, 369-369, 371-371, 385-385


42-346: 🛠️ Refactor suggestion

Extract common test setup into reusable functions

The question creation workflow has significant code duplication across different question types. Consider extracting common setup steps into reusable functions.

Create helper functions for common operations:

const createQuestion = (type: string, title: string, text: string) => {
  cy.get('[data-cy="create-question"]').click()
  cy.get('[data-cy="select-question-type"]').click()
  cy.get(`[data-cy="select-question-type-${type}"]`).click()
  cy.get('[data-cy="insert-question-title"]').type(title)
  cy.get('[data-cy="insert-question-text"]').realClick().type(text)
}

const setQuestionStatus = (status: string) => {
  cy.get('[data-cy="select-question-status"]').click()
  cy.get(`[data-cy="select-question-status-${status}"]`).click()
}

Copy link

sonarcloud bot commented Oct 29, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
8.7% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

1 participant