Programming exercises: Fix an issue where the result export does not work with structured grading criteria
#10174
Conversation
github-actions
bot
added
tests
client
chrisknedl
added
programming
chrisknedl
changed the title
Programming exercises: Fix an issue where the result export did not work with structured grading criteriaProgramming exercises: Fix an issue where the result export does not work with structured grading criteria
WalkthroughThe pull request modifies the handling of the Changes
Assessment against linked issues
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 (7)
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: 1
🧹 Nitpick comments (3)
src/main/webapp/app/entities/result-with-points-per-grading-criterion.model.ts (1)
9-10: LGTM! Consider using a type alias for better maintainability.The union type correctly handles both JSON objects and Maps, addressing the deserialization issue. The comment clearly explains the rationale.
Consider extracting the union type to a type alias for better reusability:
+type PointsPerCriterion = { [key: string]: number } | Map<GradingCriterionId, Points>; + export class ResultWithPointsPerGradingCriterion { result: Result; totalPoints: Points; - pointsPerCriterion: { [key: string]: number } | Map<GradingCriterionId, Points>; + pointsPerCriterion: PointsPerCriterion; }src/main/webapp/app/exercises/shared/result/result.service.ts (1)
263-264: LGTM! Consider adding error handling for invalid keys.The change correctly handles the JSON format from the server. The use of
Object.entries()and the unary plus operator for key conversion is appropriate.Consider adding validation for key conversion:
Object.entries(resultWithPoints.pointsPerCriterion).forEach(([key, value]) => { - pointsMap.set(+key, value); + const numericKey = +key; + if (Number.isFinite(numericKey)) { + pointsMap.set(numericKey, value); + } else { + console.warn(`Invalid criterion ID: ${key}`); + } });src/test/javascript/spec/service/result.service.spec.ts (1)
158-158: LGTM! Consider adding edge case tests.The test data correctly reflects the JSON format received from the server. The verification of the conversion to Map is appropriate.
Consider adding test cases for edge cases:
// Test empty object resultWithPoints3.pointsPerCriterion = {}; // Test invalid keys resultWithPoints4.pointsPerCriterion = { 'invalid': 20, '2': 30 }; // Test invalid values resultWithPoints5.pointsPerCriterion = { '1': null, '2': undefined };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/webapp/app/entities/result-with-points-per-grading-criterion.model.ts(1 hunks)src/main/webapp/app/exercises/shared/exercise-scores/exercise-scores-export-button.component.ts(1 hunks)src/main/webapp/app/exercises/shared/result/result.service.ts(1 hunks)src/test/javascript/spec/service/result.service.spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
src/test/javascript/spec/service/result.service.spec.ts (1)
Pattern src/test/javascript/spec/**/*.ts: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}
src/main/webapp/app/exercises/shared/exercise-scores/exercise-scores-export-button.component.ts (1)
src/main/webapp/app/entities/result-with-points-per-grading-criterion.model.ts (1)
src/main/webapp/app/exercises/shared/result/result.service.ts (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Codacy Static Code Analysis
src/main/webapp/app/exercises/shared/exercise-scores/exercise-scores-export-button.component.ts
Outdated
Show resolved
Hide resolved
WalkthroughThe pull request addresses an issue with result export functionality by modifying the handling of Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
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
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/main/webapp/app/entities/result-with-points-per-grading-criterion.model.ts (1)
9-10: LGTM! Consider enhancing the comment.The type union effectively addresses the deserialization issue. The comment explains the reason for the change, but could be more specific about the data flow.
Consider expanding the comment to be more specific:
- // type union is needed here, because deserialized java maps are actually represented as plain JSON key-value pairs + // Type union is needed here because Java maps are deserialized from the server as plain JSON key-value pairs instead of JavaScript Map objectssrc/main/webapp/app/exercises/shared/result/result.service.ts (1)
263-264: LGTM! Consider adding error handling.The conversion from plain object to Map is implemented correctly. However, it might be good to add error handling for invalid numeric keys.
Consider adding validation:
Object.entries(resultWithPoints.pointsPerCriterion).forEach(([key, value]) => { + const numericKey = +key; + if (Number.isNaN(numericKey)) { + console.warn(`Invalid numeric key in pointsPerCriterion: ${key}`); + return; + } - pointsMap.set(+key, value); + pointsMap.set(numericKey, value); });src/test/javascript/spec/service/result.service.spec.ts (1)
158-158: LGTM! Consider adding more test cases.The test data now correctly reflects the server response format. However, we could improve test coverage.
Consider adding these test cases:
- Invalid numeric keys:
{ 'abc': 20 }- Empty object:
{}- Null/undefined values:
{ '1': null }- Already a Map (edge case):
new Map([[1, 20]])
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/webapp/app/entities/result-with-points-per-grading-criterion.model.ts(1 hunks)src/main/webapp/app/exercises/shared/exercise-scores/exercise-scores-export-button.component.ts(1 hunks)src/main/webapp/app/exercises/shared/result/result.service.ts(1 hunks)src/test/javascript/spec/service/result.service.spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
src/main/webapp/app/exercises/shared/exercise-scores/exercise-scores-export-button.component.ts (1)
src/test/javascript/spec/service/result.service.spec.ts (1)
Pattern src/test/javascript/spec/**/*.ts: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}
src/main/webapp/app/entities/result-with-points-per-grading-criterion.model.ts (1)
src/main/webapp/app/exercises/shared/result/result.service.ts (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Codacy Static Code Analysis
src/main/webapp/app/exercises/shared/exercise-scores/exercise-scores-export-button.component.ts
Outdated
Show resolved
Hide resolved
Feras797
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
GitHub Actions
Inactive
github-actions
bot
removed
assessment
|
github-actions
bot
added
the
deployment-error
github-actions
bot
removed
the
deployment-error
HawKhiem
temporarily deployed
to
artemis-test5.artemis.cit.tum.de
GitHub Actions
Inactive
|
Tested on TS5 Worked as expected |
Checklist
General
Client
Motivation and Context
Closes #10000
Description
The client assumed that the Java map is converted to a proper JS map. However, on deserialization, the map still is still in JSON format with key-value pairs. We first have to create a map with
Object.entries()so that we can then call theMap.forEach()function. The test was also adapted to use the format that is actually received from the server.Steps for 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
Code Review
Manual Tests
Test Coverage
Screenshots
Summary by CodeRabbit
Type Changes
pointsPerCriterionproperty to support both Map and object representations.Type Safety Improvements
Test Updates
pointsPerCriterion, simplifying the data structure representation.