Assessment: Do not display automatic as additional feedback in the online code editor
#10120
Conversation
…unreferenced feedback filtering
github-actions
bot
added
tests
client
WalkthroughThis pull request introduces changes to the feedback retrieval mechanism across multiple exercise components. The primary modification involves replacing the generic Changes
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/test/javascript/spec/component/utils/result.utils.spec.ts (1)
23-24: Test case needs additional coverage.While the test case correctly verifies manual unreferenced feedback filtering, it should be expanded to cover more scenarios:
- Empty feedback array
- Array with only automatic unreferenced feedback
- Array with mixed manual and automatic unreferenced feedback
- it('should filter out all non unreferenced feedbacks', () => { - const feedbacks = [{ reference: 'foo' }, { reference: 'foo', type: FeedbackType.MANUAL_UNREFERENCED }, { type: FeedbackType.MANUAL_UNREFERENCED }, {}]; - const unreferencedFeedbacks = getManualUnreferencedFeedback(feedbacks); - expect(unreferencedFeedbacks).toEqual([{ type: FeedbackType.MANUAL_UNREFERENCED }]); - }); + describe('getManualUnreferencedFeedback', () => { + it('should return undefined for undefined input', () => { + expect(getManualUnreferencedFeedback(undefined)).toBeUndefined(); + }); + + it('should return empty array for empty input', () => { + expect(getManualUnreferencedFeedback([])).toEqual([]); + }); + + it('should filter out all non-manual unreferenced feedbacks', () => { + const feedbacks = [ + { reference: 'foo' }, + { reference: 'foo', type: FeedbackType.MANUAL_UNREFERENCED }, + { type: FeedbackType.MANUAL_UNREFERENCED }, + { type: FeedbackType.AUTOMATIC }, + {} + ]; + expect(getManualUnreferencedFeedback(feedbacks)).toEqual([ + { type: FeedbackType.MANUAL_UNREFERENCED } + ]); + }); + + it('should handle array with only automatic unreferenced feedback', () => { + const feedbacks = [ + { type: FeedbackType.AUTOMATIC }, + { type: FeedbackType.AUTOMATIC } + ]; + expect(getManualUnreferencedFeedback(feedbacks)).toEqual([]); + }); + });src/main/webapp/app/exercises/shared/result/result.utils.ts (1)
119-133: Consider enhancing error handling and input validation.While the implementation is correct, consider adding input validation for edge cases:
- Handle malformed feedback objects (missing type property)
- Add type guards for better type safety
+interface ValidFeedback extends Feedback { + type: FeedbackType; +} + +function isValidFeedback(feedback: Feedback): feedback is ValidFeedback { + return 'type' in feedback; +} + export const getManualUnreferencedFeedback = (feedbacks: Feedback[] | undefined): Feedback[] | undefined => { - return feedbacks ? feedbacks.filter((feedbackElement) => !feedbackElement.reference && feedbackElement.type === FeedbackType.MANUAL_UNREFERENCED) : undefined; + return feedbacks?.filter((feedbackElement): feedbackElement is ValidFeedback => + isValidFeedback(feedbackElement) && !feedbackElement.reference && feedbackElement.type === FeedbackType.MANUAL_UNREFERENCED + ); }; export const getAutomaticUnreferencedFeedback = (feedbacks: Feedback[] | undefined): Feedback[] | undefined => { - return feedbacks ? feedbacks.filter((feedbackElement) => !feedbackElement.reference && feedbackElement.type === FeedbackType.AUTOMATIC) : undefined; + return feedbacks?.filter((feedbackElement): feedbackElement is ValidFeedback => + isValidFeedback(feedbackElement) && !feedbackElement.reference && feedbackElement.type === FeedbackType.AUTOMATIC + ); };src/main/webapp/app/exercises/modeling/participate/modeling-submission.component.ts (1)
666-668: Consider documenting the feedback type selection rationale.The codebase now uses different types of feedback for different exercise types:
- Text exercises: Manual feedback
- Modeling exercises: Automatic feedback
Consider adding documentation to explain this architectural decision, making it easier for future developers to understand why different feedback types are used for different exercise types.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/main/webapp/app/exercises/file-upload/participate/file-upload-submission.component.ts(2 hunks)src/main/webapp/app/exercises/modeling/participate/modeling-submission.component.ts(2 hunks)src/main/webapp/app/exercises/programming/participate/code-editor-student-container.component.ts(2 hunks)src/main/webapp/app/exercises/shared/result/result.utils.ts(1 hunks)src/main/webapp/app/exercises/text/participate/text-editor.component.ts(2 hunks)src/test/javascript/spec/component/utils/result.utils.spec.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
src/main/webapp/app/exercises/file-upload/participate/file-upload-submission.component.ts (1)
src/main/webapp/app/exercises/programming/participate/code-editor-student-container.component.ts (1)
src/test/javascript/spec/component/utils/result.utils.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/text/participate/text-editor.component.ts (1)
src/main/webapp/app/exercises/modeling/participate/modeling-submission.component.ts (1)
src/main/webapp/app/exercises/shared/result/result.utils.ts (1)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: client-tests-selected
- GitHub Check: client-tests
- GitHub Check: Build and Push Docker Image
- GitHub Check: server-tests
- GitHub Check: Build .war artifact
- GitHub Check: Analyse
🔇 Additional comments (11)
src/main/webapp/app/exercises/programming/participate/code-editor-student-container.component.ts (2)
17-17: LGTM! Import statement updated correctly.The import statement has been updated to use the new
getManualUnreferencedFeedbackfunction, which aligns with the PR objective of separating manual and automatic feedback.
163-163: LGTM! Getter implementation updated correctly.The
unreferencedFeedbackgetter now specifically retrieves manual unreferenced feedback, which is the correct behavior for this component as it should only display manual feedback to students.src/test/javascript/spec/component/utils/result.utils.spec.ts (1)
4-4: LGTM! Import statement updated correctly.The import statement has been updated to include the new
getManualUnreferencedFeedbackfunction.src/main/webapp/app/exercises/file-upload/participate/file-upload-submission.component.ts (2)
25-25: LGTM! Import statement updated correctly.The import statement has been updated to use the new
getManualUnreferencedFeedbackfunction.
250-250: LGTM! Getter implementation updated correctly.The
unreferencedFeedbackgetter now specifically retrieves manual unreferenced feedback, which is the correct behavior for this component.src/main/webapp/app/exercises/shared/result/result.utils.ts (2)
119-125: LGTM! Manual feedback utility function implemented correctly.The
getManualUnreferencedFeedbackfunction is well-implemented with clear documentation and proper type safety.
127-133: LGTM! Automatic feedback utility function implemented correctly.The
getAutomaticUnreferencedFeedbackfunction is well-implemented with clear documentation and proper type safety.src/main/webapp/app/exercises/text/participate/text-editor.component.ts (2)
23-23: LGTM! Import statement updated to use the new feedback utility function.The change aligns with the PR objectives to separate manual and automatic feedback retrieval.
315-315: LGTM! Updated to use manual feedback retrieval.The getter now correctly returns only manual unreferenced feedback, which is appropriate for text exercises.
src/main/webapp/app/exercises/modeling/participate/modeling-submission.component.ts (2)
18-18: LGTM! Import statement updated to use the new feedback utility function.The change aligns with the PR objectives to separate manual and automatic feedback retrieval.
667-667: LGTM! Updated to use automatic feedback retrieval.The getter now correctly returns only automatic unreferenced feedback, which is appropriate for modeling exercises.
github-actions
bot
added
the
exercise
LeonWehrhahn
changed the title
General: Refactor feedback utility functions to separate manual and automaticGeneral: Refactor feedback utility functions to separate manual and automatic
vinceclifford
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
|
github-actions
bot
added
the
deployment-error
There was a problem hiding this comment.
Tested on TS1.
-
No Additional Feedback is shown
-
I'm not quite sure how to prompt Athena to generate a feedback, should we also tick the checkbox "Allow automatic AI preliminary feedback requests"? I did that and try to request AI feedback and got the following error
We are currently having difficulties with Athena TS1; normally, having "Allow automatic Al preliminary feedback requests" checked enables student feedback generation. However, Athena TS1 is currently not working - so feedback won't be generated on this test server. However, the following tests, should verify that also unreferenced AUTOMATIC Feedback is still working for modeling exercises. Please take a look at those and verify that they are correct - as an alternative check, as Athena TS1 will probably need a couple of days till its running again. result.utils.spec.ts:
modeling-submission.component.spec.ts:
modeling-submission-team.component.spec.ts
|
…ub.com/ls1intum/Artemis into bugfix/unreferenced-feedback-filtering
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/test/javascript/spec/component/utils/result.utils.spec.ts (2)
22-32: Consider enhancing test coverage with additional edge casesThe test case effectively verifies the basic functionality of
getManualUnreferencedFeedback. However, consider adding:
- Edge cases for null/undefined feedbacks
- Type assertions for the test data
it('should filter out all non unreferenced feedbacks that do not have type MANUAL_UNREFERENCED', () => { const feedbacks = [ { reference: 'foo' }, { reference: 'foo', type: FeedbackType.MANUAL_UNREFERENCED }, { type: FeedbackType.AUTOMATIC }, { type: FeedbackType.MANUAL_UNREFERENCED }, {}, + null, + undefined, ] as Feedback[]; const unreferencedFeedbacks = getManualUnreferencedFeedback(feedbacks); - expect(unreferencedFeedbacks).toEqual([{ type: FeedbackType.MANUAL_UNREFERENCED }]); + expect(unreferencedFeedbacks).toContainEqual<Feedback>({ type: FeedbackType.MANUAL_UNREFERENCED }); + expect(unreferencedFeedbacks).toHaveLength(1); });
34-44: Apply similar enhancements to automatic feedback testFor consistency with the manual feedback test improvements, consider enhancing this test case with the same patterns:
- Edge cases for null/undefined feedbacks
- Type assertions for the test data
it('should filter out all non unreferenced feedbacks that do not have type AUTOMATIC', () => { const feedbacks = [ { reference: 'foo' }, { reference: 'foo', type: FeedbackType.AUTOMATIC }, { type: FeedbackType.AUTOMATIC }, { type: FeedbackType.MANUAL_UNREFERENCED }, {}, + null, + undefined, ] as Feedback[]; const unreferencedFeedbacks = getAutomaticUnreferencedFeedback(feedbacks); - expect(unreferencedFeedbacks).toEqual([{ type: FeedbackType.AUTOMATIC }]); + expect(unreferencedFeedbacks).toContainEqual<Feedback>({ type: FeedbackType.AUTOMATIC }); + expect(unreferencedFeedbacks).toHaveLength(1); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/test/javascript/spec/component/utils/result.utils.spec.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/test/javascript/spec/component/utils/result.utils.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}}
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: client-tests-selected
- GitHub Check: client-tests
- GitHub Check: server-tests
- GitHub Check: Build and Push Docker Image
- GitHub Check: Build .war artifact
- GitHub Check: Analyse
🔇 Additional comments (2)
src/test/javascript/spec/component/utils/result.utils.spec.ts (2)
4-5: LGTM! Clear and well-organized importsThe new imports for
getAutomaticUnreferencedFeedbackandgetManualUnreferencedFeedbackare properly organized and clearly named.
22-44: Verify test coverage aligns with PR objectivesThe test suite effectively covers the separation of manual and automatic unreferenced feedback, which aligns with the PR's main objective. The test cases verify that:
- Manual unreferenced feedback is correctly filtered
- Automatic unreferenced feedback is correctly filtered
This satisfies the requirement to "verify the correctness of the unreferenced AUTOMATIC feedback" as mentioned in the PR objectives.
✅ Verification successful
Test coverage is comprehensive and aligns with implementation
The test cases adequately cover the functionality of both feedback filtering functions, matching their actual usage across multiple components in the codebase. The tests verify all essential scenarios including mixed feedback types, referenced/unreferenced items, and empty objects. The implementation is straightforward, and no edge cases are left uncovered.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify test coverage for feedback-related functions jest --coverage --collectCoverageFrom="**/result.utils.ts" --testPathPattern="result.utils.spec.ts"Length of output: 194
Script:
#!/bin/bash # Find the implementation file fd result.utils.ts # Get the implementation of feedback filtering functions ast-grep --pattern 'export function getManualUnreferencedFeedback($_) { $$$ }' ast-grep --pattern 'export function getAutomaticUnreferencedFeedback($_) { $$$ }' # Look for other usages of these functions rg "getManualUnreferencedFeedback|getAutomaticUnreferencedFeedback" -A 2Length of output: 5829
krusche
changed the title
General: Refactor feedback utility functions to separate manual and automaticAssessment: Do not display automatic as manual feedback in the online code editor
krusche
changed the title
Assessment: Do not display automatic as manual feedback in the online code editorAssessment: Do not display automatic as additional feedback in the online code editor
Checklist
General
Client
Changes affecting Programming Exercises
Motivation and Context
The previous implementation of
getUnreferencedFeedback()returned unreferenced feedback of typeMANUAL_UNREFERENCEDand of typeAUTOMATIC. Since some parts of the application expected to only receive unreferenced feedback of typeMANUAL_UNREFERENCED,AUTOMATICunreferenced feedback was displayed in places where it was not expected.Description
This PR introduces two new functions:
MANUAL_UNREFERENCEDAUTOMATICThe code was updated accordingly to use the appropriate functions.
Steps for Testing
Prerequisites:
Instructor:
Student: Programming
Additional Feedbackshown below theBuild Output, or that ifAdditional Feedbackis shown, the feedback only includes non-automatic feedback (no test case feedback)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
Screenshots
UI before PR changes
The unreferenced Automatic Feedback (In this case, test cases) is displayed in the
Additional Feedbacksection, which is not the intended behavior.UI after PR changes
The unreferenced Automatic Feedback is now not displayed anymore in the
Additional Feedbacksection.Summary by CodeRabbit
Refactor
getUnreferencedFeedbackwith more specificgetManualUnreferencedFeedbackandgetAutomaticUnreferencedFeedbackfunctions.Tests