Quiz exercises: Allow decimal point values for quiz questions
#10232
Conversation
github-actions
bot
added
server
badkeyy
changed the title
Quiz exercises: Add decimal points to quiz exercise
badkeyy
changed the title
Quiz exercises: Add decimal points to quiz exerciseQuiz exercises: Allow decimal point values for quiz questions
github-actions
bot
added
tests
exam
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 PMD (7.8.0)src/main/java/de/tum/cit/aet/artemis/quiz/domain/QuizQuestion.javaThe following rules are missing or misspelled in your ruleset file category/vm/bestpractices.xml: BooleanInstantiation, DontImportJavaLang, DuplicateImports, EmptyFinallyBlock, EmptyIfStmt, EmptyInitializer, EmptyStatementBlock, EmptyStatementNotInLoop, EmptySwitchStatements, EmptySynchronizedBlock, EmptyTryBlock, EmptyWhileStmt, ExcessiveClassLength, ExcessiveMethodLength, ImportFromSamePackage, MissingBreakInSwitch, SimplifyBooleanAssertion. Please check your ruleset configuration. WalkthroughThe pull request introduces comprehensive modifications to the quiz scoring system across multiple files. The primary change involves transitioning from integer-based points to double-based points, allowing for more granular scoring. This affects the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
src/main/java/de/tum/cit/aet/artemis/quiz/domain/QuizQuestion.java (1)
71-72: Consider using BigDecimal for precise decimal calculations.While using
Doubleallows for fractional points as intended, it might lead to precision issues in decimal arithmetic. Consider usingBigDecimalif exact decimal representation is crucial for point calculations.- private Double points; + private BigDecimal points;src/main/webapp/app/exercises/quiz/shared/quiz-manage-util.service.ts (1)
40-42: Improve type safety in points validation.The condition handles undefined check correctly but could be more explicit about null checks and type coercion.
- if (question.points == undefined || question.points <= 0 || question.points > MAX_QUIZ_QUESTION_POINTS) { + if (question.points === undefined || question.points === null || + Number(question.points) <= 0 || Number(question.points) > MAX_QUIZ_QUESTION_POINTS) {src/test/java/de/tum/cit/aet/artemis/quiz/util/QuizExerciseFactory.java (1)
444-444: Consider adding test cases with decimal scores.While the conversion to double literals is correct, consider adding test factory methods that create questions with decimal point scores (e.g., 2.5, 3.5) to thoroughly test the new functionality.
Also applies to: 520-520
src/test/java/de/tum/cit/aet/artemis/quiz/QuizSubmissionIntegrationTest.java (1)
211-212: Consider adding test cases for edge cases in partial scoring.The test covers basic partial scoring, but could benefit from additional test cases:
- Scores very close to 0.5 (rounding boundary)
- Maximum allowed decimal places
quizExercise.getQuizQuestions().get(1).score(1d); quizExercise.getQuizQuestions().get(2).score(1d); +// Add edge cases +quizExercise.getQuizQuestions().get(1).score(0.49d); // Should round down +quizExercise.getQuizQuestions().get(2).score(0.51d); // Should round up
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
src/main/resources/config/liquibase/changelog/20250129173003_changelog.xmlis excluded by!**/*.xmlsrc/main/resources/config/liquibase/master.xmlis excluded by!**/*.xml
📒 Files selected for processing (9)
src/main/java/de/tum/cit/aet/artemis/quiz/domain/QuizQuestion.java(3 hunks)src/main/webapp/app/exercises/quiz/manage/drag-and-drop-question/drag-and-drop-question-edit.component.html(1 hunks)src/main/webapp/app/exercises/quiz/manage/multiple-choice-question/multiple-choice-question-edit.component.html(1 hunks)src/main/webapp/app/exercises/quiz/manage/short-answer-question/short-answer-question-edit.component.html(1 hunks)src/main/webapp/app/exercises/quiz/shared/quiz-manage-util.service.ts(2 hunks)src/test/java/de/tum/cit/aet/artemis/exam/service/ExamQuizServiceTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/quiz/QuizExerciseIntegrationTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/quiz/QuizSubmissionIntegrationTest.java(5 hunks)src/test/java/de/tum/cit/aet/artemis/quiz/util/QuizExerciseFactory.java(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/test/java/de/tum/cit/aet/artemis/quiz/QuizExerciseIntegrationTest.java
🧰 Additional context used
📓 Path-based instructions (8)
src/main/webapp/app/exercises/quiz/manage/multiple-choice-question/multiple-choice-question-edit.component.html (1)
Pattern src/main/webapp/**/*.html: @if and @for are new and valid Angular syntax replacing *ngIf and *ngFor. They should always be used over the old style.
src/main/webapp/app/exercises/quiz/manage/drag-and-drop-question/drag-and-drop-question-edit.component.html (1)
Pattern src/main/webapp/**/*.html: @if and @for are new and valid Angular syntax replacing *ngIf and *ngFor. They should always be used over the old style.
src/main/webapp/app/exercises/quiz/shared/quiz-manage-util.service.ts (1)
src/test/java/de/tum/cit/aet/artemis/exam/service/ExamQuizServiceTest.java (1)
Pattern src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true
src/test/java/de/tum/cit/aet/artemis/quiz/QuizSubmissionIntegrationTest.java (1)
Pattern src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true
src/main/webapp/app/exercises/quiz/manage/short-answer-question/short-answer-question-edit.component.html (1)
Pattern src/main/webapp/**/*.html: @if and @for are new and valid Angular syntax replacing *ngIf and *ngFor. They should always be used over the old style.
src/main/java/de/tum/cit/aet/artemis/quiz/domain/QuizQuestion.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/test/java/de/tum/cit/aet/artemis/quiz/util/QuizExerciseFactory.java (1)
Pattern src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Call Build Workflow / Build .war artifact
- GitHub Check: Call Build Workflow / Build and Push Docker Image
- GitHub Check: client-tests
- GitHub Check: server-tests
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Analyse
🔇 Additional comments (9)
src/main/java/de/tum/cit/aet/artemis/quiz/domain/QuizQuestion.java (1)
144-155: Update method signatures to match field type change.The getter, setter, and score methods have been correctly updated to use Double. However, if you decide to use BigDecimal as suggested above, these methods would need to be updated accordingly.
src/main/webapp/app/exercises/quiz/shared/quiz-manage-util.service.ts (1)
Line range hint
128-133: LGTM! Error message handling is consistent.The error message handling for invalid points is properly updated to match the new validation logic.
src/test/java/de/tum/cit/aet/artemis/quiz/util/QuizExerciseFactory.java (3)
107-107: LGTM! Consistent use of double literal.The score value is correctly updated to use the double literal
2d.
153-153: LGTM! Consistent use of double literal.The score value is correctly updated to use the double literal
3d.
225-225: LGTM! Consistent use of double literal.The score value is correctly updated to use the double literal
4d.src/test/java/de/tum/cit/aet/artemis/quiz/QuizSubmissionIntegrationTest.java (2)
354-356: LGTM!The practice mode test correctly handles double-based scores and rounding.
164-166: Verify the rounding behavior in score calculations.The change from integer to double-based score calculation requires careful handling of rounding. Ensure that:
- The rounding behavior is consistent across all score calculations
- The size of point counters array matches the rounded score value
✅ Verification successful
Rounding behavior in score calculations is consistent and correct
The rounding implementation is uniform across all score calculations, with proper decimal handling for:
- Point counter array sizing using
(int) Math.round(score + 1)- Student count calculations using decimal division before rounding
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other score calculations to verify consistent rounding rg -A 2 "Math\.round" src/test/java/de/tum/cit/aet/artemis/quiz/Length of output: 5885
src/main/webapp/app/exercises/quiz/manage/short-answer-question/short-answer-question-edit.component.html (1)
28-31: LGTM! Consistent with other question types.The changes properly implement decimal point support with appropriate step value and minimum bound.
src/main/webapp/app/exercises/quiz/manage/drag-and-drop-question/drag-and-drop-question-edit.component.html (1)
24-33: LGTM! Implements decimal points consistently.The implementation matches other question types and properly handles:
- Zero-point minimum
- Half-point increments
- Change detection
src/main/java/de/tum/cit/aet/artemis/quiz/domain/QuizQuestion.java
Outdated
Show resolved
Hide resolved
src/test/java/de/tum/cit/aet/artemis/exam/service/ExamQuizServiceTest.java
Outdated
Show resolved
Hide resolved
.../exercises/quiz/manage/multiple-choice-question/multiple-choice-question-edit.component.html
Show resolved
Hide resolved
…tion/multiple-choice-question-edit.component.html Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
vinceclifford
temporarily deployed
to
artemis-test5.artemis.cit.tum.de
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/main/java/de/tum/cit/aet/artemis/quiz/domain/QuizQuestion.java (2)
144-155: Add validation and conversion logic for points.Consider adding validation to ensure points are non-negative and within a reasonable range. Also, consider handling precision to avoid floating-point representation issues.
public Double getPoints() { return points; } public QuizQuestion score(Double score) { + if (score != null) { + // Round to 2 decimal places to avoid floating-point precision issues + score = Math.round(score * 100.0) / 100.0; + // Ensure non-negative value + score = Math.max(0.0, score); + } this.points = score; return this; } public void setPoints(Double score) { + if (score != null) { + // Round to 2 decimal places to avoid floating-point precision issues + score = Math.round(score * 100.0) / 100.0; + // Ensure non-negative value + score = Math.max(0.0, score); + } this.points = score; }
Line range hint
223-231: Consider precision handling in score comparison.The
isAnswerCorrectmethod directly compares the calculated score withgetPoints(). With decimal values, consider adding a small epsilon or usingDouble.compare()for more reliable comparison:public boolean isAnswerCorrect(SubmittedAnswer submittedAnswer) { - return makeScoringStrategy().calculateScore(this, submittedAnswer) == getPoints(); + double calculatedScore = makeScoringStrategy().calculateScore(this, submittedAnswer); + return Math.abs(calculatedScore - getPoints()) < 0.0001; // Use small epsilon for floating-point comparison }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/de/tum/cit/aet/artemis/quiz/domain/QuizQuestion.java(3 hunks)src/test/java/de/tum/cit/aet/artemis/exam/service/ExamQuizServiceTest.java(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/main/java/de/tum/cit/aet/artemis/quiz/domain/QuizQuestion.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/test/java/de/tum/cit/aet/artemis/exam/service/ExamQuizServiceTest.java (1)
Pattern src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true
🔇 Additional comments (3)
src/test/java/de/tum/cit/aet/artemis/exam/service/ExamQuizServiceTest.java (1)
365-367: Add test cases for decimal point values.While the type changes and floating-point comparison are correct, consider adding specific test cases that verify:
- Fractional points (e.g., 0.5, 1.5)
- Edge cases (e.g., very small decimals)
- Rounding behavior
Also, consider extracting the delta value to a constant for better maintainability:
+ private static final double SCORE_COMPARISON_DELTA = 0.0001; + private void checkStatistics(QuizExercise quizExercise) { // ... double questionScore = quizExerciseWithStatistic.getQuizQuestions().stream() .map(QuizQuestion::getPoints).reduce(0.0, Double::sum); - assertThat(quizExerciseWithStatistic.getMaxPoints()).isCloseTo(questionScore, within(0.0001)); + assertThat(quizExerciseWithStatistic.getMaxPoints()) + .isCloseTo(questionScore, within(SCORE_COMPARISON_DELTA));src/main/java/de/tum/cit/aet/artemis/quiz/domain/QuizQuestion.java (2)
260-260: Proper implementation of double comparison.The implementation correctly uses
Double.compare()for floating-point comparison, addressing potential precision issues. This is a good practice for handling decimal values.
72-72: Verify database migration for points column type change.The change from
IntegertoDoublerequires a database migration. While the PR objectives mention a migration exists, please ensure it handles:
- Data type conversion from INTEGER to DOUBLE
- Existing data transformation
- Proper column type definition for precision/scale
✅ Verification successful
Database migration for points column type change is properly implemented
The migration exists in
20250129173003_changelog.xmland correctly modifies thepointscolumn type todoublein thequiz_questiontable. The change is properly structured and will handle the data type conversion automatically.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if a migration exists for the points column type change # Look for migration files containing 'points' or 'quiz_question' fd -e sql -e java -e xml . | rg -i "migration|changelog" | xargs rg -l -i "points|quiz_question"Length of output: 253
Script:
#!/bin/bash # Check the content of the recent changelog cat src/main/resources/config/liquibase/changelog/20250129173003_changelog.xmlLength of output: 600
.../exercises/quiz/manage/multiple-choice-question/multiple-choice-question-edit.component.html
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/quiz/domain/QuizQuestion.java
Outdated
Show resolved
Hide resolved
zagemello
temporarily deployed
to
artemis-test4.artemis.cit.tum.de
GitHub Actions
Inactive
Feras797
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
alekspetrov9e
temporarily deployed
to
artemis-test4.artemis.cit.tum.de
GitHub Actions
Inactive
SindiBuklaji
temporarily deployed
to
artemis-test5.artemis.cit.tum.de
GitHub Actions
Inactive
kevinfischer4
requested a deployment
to
artemis-test2.artemis.cit.tum.de
GitHub Actions
In progress
🚨 Contains Database Migration 🚨
Checklist
General
Server
Client
authoritiesto all new routes and checked the course groups for displaying navigation elements (links, buttons).Motivation and Context
Currently it is not possible to award fractions of a point for quiz questions. Issue #10148 asked to change that.
Steps for Testing
Prerequisites:
Exam Mode Testing
Prerequisites:
Testserver States
Note
These badges show the state of the test servers.
Green = Currently available, Red = Currently locked
Click on the badges to get to the test servers.
Review Progress
Performance Review
Code Review
Manual Tests
Exam Mode Test
Performance Tests
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
These changes provide more granular and precise scoring options for quiz questions, allowing for more nuanced evaluation of student performance.