Text exercises: Replace feedback modal with inline feedback view
#9395
Conversation
…, add text blocks
github-actions
bot
added
deployment-error
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
EneaGore
added
deploy:artemis-test1
and removed
deployment-error
EneaGore
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/test/javascript/spec/component/overview/exercise-details/request-feedback-button/request-feedback-button.component.spec.ts (2)
218-219: Enhance test specificity with additional assertions.While the test correctly verifies the button state, consider adding assertions to explicitly verify that both conditions influence the button's enabled state:
expect(component.isSubmitted).toBeTrue(); expect(component.isGeneratingFeedback).toBeFalse();This makes it clearer which conditions are being tested and helps with debugging if the test fails.
Line range hint
1-230: Add test coverage for missing scenarios.The test suite is missing coverage for:
- The
pendingChangesinput property's effect on the button state- The case when
isGeneratingFeedbackis true butisSubmittedis also trueConsider adding these test cases:
it('should disable the button when there are pending changes', fakeAsync(() => { setAthenaEnabled(true); const participation = { id: 1, submissions: [{ id: 1, submitted: true }], testRun: false, } as StudentParticipation; const exercise = { id: 1, type: ExerciseType.TEXT, studentParticipations: [participation], course: {}, allowFeedbackRequests: true } as Exercise; fixture.componentRef.setInput('exercise', exercise); fixture.componentRef.setInput('isSubmitted', true); fixture.componentRef.setInput('isGeneratingFeedback', false); fixture.componentRef.setInput('pendingChanges', true); mockExerciseDetails(exercise); component.ngOnInit(); tick(); fixture.detectChanges(); const button = debugElement.query(By.css('button')); expect(button.nativeElement.disabled).toBeTrue(); expect(component.pendingChanges).toBeTrue(); })); it('should disable the button when feedback is being generated regardless of submission state', fakeAsync(() => { setAthenaEnabled(true); const participation = { id: 1, submissions: [{ id: 1, submitted: true }], testRun: false, } as StudentParticipation; const exercise = { id: 1, type: ExerciseType.TEXT, studentParticipations: [participation], course: {}, allowFeedbackRequests: true } as Exercise; fixture.componentRef.setInput('exercise', exercise); fixture.componentRef.setInput('isSubmitted', true); fixture.componentRef.setInput('isGeneratingFeedback', true); mockExerciseDetails(exercise); component.ngOnInit(); tick(); fixture.detectChanges(); const button = debugElement.query(By.css('button')); expect(button.nativeElement.disabled).toBeTrue(); expect(component.isGeneratingFeedback).toBeTrue(); }));
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- src/test/javascript/spec/component/overview/exercise-details/request-feedback-button/request-feedback-button.component.spec.ts (2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/test/javascript/spec/component/overview/exercise-details/request-feedback-button/request-feedback-button.component.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}}
🔇 Additional comments (1)
src/test/javascript/spec/component/overview/exercise-details/request-feedback-button/request-feedback-button.component.spec.ts (1)
196-197: Past review comment is still applicable.The existing review comment about splitting this test case into multiple test cases for better coverage is still valid.
FelixTJDietrich
added
maintainer-approved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
src/main/webapp/app/exercises/text/participate/text-editor.component.ts (1)
Line range hint
385-388: Enhance error handling in submission.The current error handling only displays the error message. Consider adding proper error recovery and state reset.
Apply this diff:
error: (err: HttpErrorResponse) => { - this.alertService.error(err.error.message); + const errorMessage = err.error?.message || this.translateService.instant('artemisApp.textExercise.error.submission'); + this.alertService.error(errorMessage); + // Reset states + this.hasAthenaResultForLatestSubmission = false; this.isSaving = false; },
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- src/main/java/de/tum/cit/aet/artemis/text/web/TextExerciseResource.java (2 hunks)
- src/main/webapp/app/exercises/text/participate/text-editor.component.ts (10 hunks)
- src/main/webapp/app/exercises/text/participate/text-editor.route.ts (1 hunks)
- src/main/webapp/i18n/de/programmingExercise.json (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/webapp/i18n/de/programmingExercise.json
🧰 Additional context used
📓 Path-based instructions (3)
src/main/java/de/tum/cit/aet/artemis/text/web/TextExerciseResource.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_importssrc/main/webapp/app/exercises/text/participate/text-editor.component.ts (1)
src/main/webapp/app/exercises/text/participate/text-editor.route.ts (1)
📓 Learnings (1)
src/main/webapp/app/exercises/text/participate/text-editor.component.ts (2)
Learnt from: iyannsch PR: ls1intum/Artemis#9379 File: src/main/webapp/app/shared/components/code-button/code-button.component.ts:143-170 Timestamp: 2024-10-08T15:35:42.972Z Learning: In `code-button.component.ts`, `this.exercise.id` is checked before use, so it's acceptable to use the non-null assertion operator `!` on `this.exercise.id!`.Learnt from: iyannsch PR: ls1intum/Artemis#9379 File: src/main/webapp/app/shared/components/code-button/code-button.component.ts:143-170 Timestamp: 2024-09-30T08:34:49.363Z Learning: In `code-button.component.ts`, `this.exercise.id` is checked before use, so it's acceptable to use the non-null assertion operator `!` on `this.exercise.id!`.
🪛 ast-grep
src/main/java/de/tum/cit/aet/artemis/text/web/TextExerciseResource.java
[warning] 432-432: Detected a cookie where the
HttpOnlyflag is either missing or disabled. TheHttpOnlycookie flag instructs the browser to forbid client-side JavaScript to read the cookie. If JavaScript interaction is required, you can ignore this finding. However, set theHttpOnlyflag to true` in all other cases.
Context: (new HashSet<>())
Note: [CWE-1004]: Sensitive Cookie Without 'HttpOnly' Flag [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration
[warning] 432-432: Detected a cookie where the
Secureflag is either missing or disabled. TheSecurecookie flag instructs the browser to forbid sending the cookie over an insecure HTTP request. Set theSecureflag totrueso the cookie will only be sent over HTTPS.
Context: (new HashSet<>())
Note: [CWE-614]: Sensitive Cookie in HTTPS Session Without 'Secure' Attribute [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration
🔇 Additional comments (4)
src/main/webapp/app/exercises/text/participate/text-editor.component.ts (1)
144-158: 🛠️ Refactor suggestionImprove readability of WebSocket subscription conditions.
The nested conditions make the code hard to read. Consider extracting the conditions into well-named variables.
Apply this diff:
+const hasNewResults = changedParticipation.results?.length > (this.participation?.results?.length || 0); +const hasUncompletedResult = changedParticipation.results?.last()?.completionDate === undefined; +const isAthenaResult = changedParticipation.results?.last()?.assessmentType === AssessmentType.AUTOMATIC_ATHENA; +const hasSuccessStatus = changedParticipation.results?.last()?.successful !== undefined; -if ( - changedParticipation.results && - ((changedParticipation.results?.length || 0) > (this.participation?.results?.length || 0) || - changedParticipation.results?.last()?.completionDate === undefined) && - changedParticipation.results?.last()?.assessmentType === AssessmentType.AUTOMATIC_ATHENA && - changedParticipation.results.last()?.successful !== undefined -) { +if (changedParticipation.results && (hasNewResults || hasUncompletedResult) && isAthenaResult && hasSuccessStatus) {Likely invalid or redundant comment.
src/main/java/de/tum/cit/aet/artemis/text/web/TextExerciseResource.java (3)
425-430: LGTM! Clear and correct implementation of result filtering.The code correctly filters results to show only AUTOMATIC_ATHENA assessments before the assessment due date, which aligns with the PR objective of displaying preliminary feedback.
435-437:⚠️ Potential issueAdd type checking before casting submission.
The current implementation directly casts
SubmissiontoTextSubmissionwithout type verification, which could lead toClassCastException.Apply this fix:
- TextSubmission textSubmission = (TextSubmission) submission; + if (!(submission instanceof TextSubmission textSubmission)) { + continue; + }Likely invalid or redundant comment.
444-445:⚠️ Potential issueUse consistent collection type for results.
The code uses
Set<Result>at line 427 butList<Result>here. For consistency and to prevent potential issues, use the same collection type.Apply this fix:
- List<Result> athenaResults = submission.getResults().stream() + Set<Result> athenaResults = submission.getResults().stream() .filter(result -> result.getAssessmentType() == AssessmentType.AUTOMATIC_ATHENA) - .toList(); + .collect(Collectors.toSet());Likely invalid or redundant comment.
krusche
deleted the
feature/text-exercises/replace-modal-with-inline-feedback
branch
Reopened from: #9310
Checklist
General
Server
Client
authoritiesto all new routes and checked the course groups for displaying navigation elements (links, buttons).Motivation and Context
The previous PR in Athena Feedback for Text Exercises added the option to enable preliminary AI Feedback for students. This PR improves the way the feedback is viewed by adding a new route. Enhanced error handling, integration tests and some minor bug fixes are also included.
Description
Create new route to view the (preliminary) results inline on the text submission. Add the timeline to that view and make results not show the modal but use that route for text exercises.
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
Performance Review
Code Review
Manual Tests
Test Coverage
✅ ❌
Client
Screenshots
Flow
H01E01.Coupling.and.Cohesion.WS24._.Test.Course.Enea.Gore.-.Google.Chrome.2024-09-14.06-37-35.mp4
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Chores