Programming exercises: Fix an issue in which long manual feedback is not correctly displayed
#9562
Conversation
github-actions
bot
added
server
…displayed' into bugfix/long-feedback-not-beeing-displayed
github-actions
bot
added
programming
WalkthroughThe changes in this pull request primarily involve the addition of a new repository, Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
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: 12
🧹 Outside diff range and nitpick comments (11)
src/main/webapp/app/exercises/shared/feedback/feedback.service.ts (1)
20-23: Add documentation and improve error handlingWhile the implementation is functionally correct, consider these improvements:
- Add JSDoc documentation to describe the method's purpose, parameters, and return value
- Consider using URL encoding for the parameters
- Add error handling for failed requests
Here's a suggested improvement:
+ /** + * Retrieves the long feedback text for a specific result and feedback combination + * @param resultId - The ID of the result + * @param feedbackId - The ID of the feedback + * @returns Promise containing the long feedback text + * @throws Error if the request fails + */ public async getLongFeedbackText(resultId: number, feedbackId: number): Promise<string> { - const url = `results/${resultId}/feedbacks/${feedbackId}/long-feedback`; + const url = `results/${encodeURIComponent(resultId)}/feedbacks/${encodeURIComponent(feedbackId)}/long-feedback`; return await this.get<string>(url, { responseType: 'text' }); }src/main/webapp/app/assessment/unreferenced-feedback-detail/unreferenced-feedback-detail.component.ts (1)
Line range hint
14-26: Consider implementing OnDestroy to prevent memory leaks.The component has multiple event emitters but lacks proper cleanup. Consider implementing the OnDestroy interface to unsubscribe from observables.
Apply this diff:
-export class UnreferencedFeedbackDetailComponent implements OnInit { +export class UnreferencedFeedbackDetailComponent implements OnInit, OnDestroy { + private destroy$ = new Subject<void>(); + + ngOnDestroy(): void { + this.destroy$.next(); + this.destroy$.complete(); + this.dialogErrorSource.complete(); + }src/test/javascript/spec/component/assessment-shared/unreferenced-feedback-detail.component.spec.ts (1)
Line range hint
48-95: Add test coverage for long feedback functionality.The current test suite lacks coverage for the long feedback functionality introduced in this PR. Consider adding the following test cases:
- Loading long feedback
- Handling long feedback display
- Error cases for long feedback retrieval
Here's a suggested test implementation:
it('should load long feedback when available', async () => { const mockFeedbackService = TestBed.inject(FeedbackService); jest.spyOn(mockFeedbackService, 'getLongFeedbackText').mockResolvedValue('long feedback text'); comp.feedback = { id: 1, hasLongFeedback: true, resultId: 123 } as Feedback; await comp.loadLongFeedback(); expect(mockFeedbackService.getLongFeedbackText).toHaveBeenCalledExactlyOnceWith(123, 1); expect(comp.feedback.detailText).toBe('long feedback text'); }); it('should handle errors when loading long feedback', async () => { const mockFeedbackService = TestBed.inject(FeedbackService); jest.spyOn(mockFeedbackService, 'getLongFeedbackText').mockRejectedValue(new Error('Failed to load')); comp.feedback = { id: 1, hasLongFeedback: true, resultId: 123 } as Feedback; await comp.loadLongFeedback(); expect(mockFeedbackService.getLongFeedbackText).toHaveBeenCalledExactlyOnceWith(123, 1); // Verify error handling behavior based on your implementation });src/main/webapp/app/exercises/shared/unreferenced-feedback/unreferenced-feedback.component.ts (3)
20-20: Add TypeScript documentation for the resultId input property.Consider adding JSDoc documentation to explain the purpose and usage of this property, especially since it's crucial for long feedback handling.
+ /** The ID of the result associated with this feedback component. Used for loading long feedback text. */ @Input() resultId: number;
Line range hint
11-13: Implement OnDestroy interface for proper cleanup.To prevent potential memory leaks, implement the OnDestroy interface to clean up any subscriptions or event listeners that might be created by external components subscribing to the EventEmitter outputs.
-export class UnreferencedFeedbackComponent { +export class UnreferencedFeedbackComponent implements OnDestroy { + ngOnDestroy() { + // Clean up any subscriptions or event listeners + this.feedbacksChange.complete(); + this.onAcceptSuggestion.complete(); + this.onDiscardSuggestion.complete(); + }
Line range hint
48-63: Improve type safety and code style compliance.A few style improvements to better align with the coding guidelines:
- validateFeedback() { + validateFeedback(): void { if (!this.unreferencedFeedback || this.unreferencedFeedback.length === 0) { this.assessmentsAreValid = false; return; } - for (const feedback of this.unreferencedFeedback) { + for (const feedback of this.unreferencedFeedback) { if (feedback.credits == undefined || isNaN(feedback.credits)) { this.assessmentsAreValid = false; return; } } this.assessmentsAreValid = true; }src/main/webapp/app/course/learning-paths/services/base-api-http.service.ts (1)
112-112: LGTM! Consider documenting responseType in JSDoc.The responseType option has been consistently added across all HTTP methods, providing the necessary flexibility for handling both JSON and text responses while maintaining backward compatibility.
Consider adding the responseType parameter to each method's JSDoc, for example:
* @param url The endpoint URL excluding the base server url (/api). * @param options The HTTP options to send with the request. + * @param options.responseType The response type to expect ('json' | 'text'). Defaults to 'json'. * @protectedAlso applies to: 144-144, 174-174, 205-205, 236-236
src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingAssessmentService.java (2)
94-97: Feedback handling changes look good, with minor optimization suggestions.The changes effectively address the long feedback display issues. Consider these improvements:
- Add null check before accessing getFeedbacks()
- Avoid unnecessary ArrayList creation
- resultService.deleteLongFeedback(newManualResult.getFeedbacks()); - List<Feedback> feedbacks = new ArrayList<>(newManualResult.getFeedbacks()); + var feedbacks = newManualResult.getFeedbacks(); + if (feedbacks != null) { + resultService.deleteLongFeedback(feedbacks); + newManualResult.updateAllFeedbackItems(feedbacks, true); + } - newManualResult.updateAllFeedbackItems(feedbacks, true);
94-94: Fix typo in comment."Assesements" should be "Assessments"
- // we do this to avoid problems of editing longFeedbackTexts in manual Assesements + // we do this to avoid problems of editing longFeedbackTexts in manual Assessmentssrc/main/java/de/tum/cit/aet/artemis/assessment/service/ResultService.java (2)
539-542: Avoid Unnecessary List Creation for Performance ImprovementIn line 541, creating a new
ArrayListfromresult.getFeedbacks()may be unnecessary ifupdateAllFeedbackItemscan work directly with the existing list.If
updateAllFeedbackItemsdoes not modify the list, consider passingresult.getFeedbacks()directly to avoid the overhead of list duplication.-List<Feedback> feedbacks = new ArrayList<>(result.getFeedbacks()); -result.updateAllFeedbackItems(feedbacks, true); +result.updateAllFeedbackItems(result.getFeedbacks(), true);Verify that this change does not affect the logic of
updateAllFeedbackItems.
15-15: Avoid Unused Imports to Clean Up the CodeLine 15 introduces an unused import
java.util.function.Function;. Unused imports can clutter the code and may lead to confusion.Remove the unused import to adhere to clean code practices.
-import java.util.function.Function;Ensure the code compiles successfully after removing the import.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (17)
- src/main/java/de/tum/cit/aet/artemis/assessment/service/AssessmentService.java (4 hunks)
- src/main/java/de/tum/cit/aet/artemis/assessment/service/ResultService.java (3 hunks)
- src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingAssessmentService.java (4 hunks)
- src/main/java/de/tum/cit/aet/artemis/text/service/TextAssessmentService.java (2 hunks)
- src/main/webapp/app/assessment/unreferenced-feedback-detail/unreferenced-feedback-detail.component.ts (3 hunks)
- src/main/webapp/app/course/learning-paths/services/base-api-http.service.ts (7 hunks)
- src/main/webapp/app/exercises/file-upload/assess/file-upload-assessment.component.html (2 hunks)
- src/main/webapp/app/exercises/modeling/assess/modeling-assessment-editor/modeling-assessment-editor.component.html (2 hunks)
- src/main/webapp/app/exercises/modeling/manage/example-modeling/example-modeling-submission.component.html (1 hunks)
- src/main/webapp/app/exercises/programming/assess/code-editor-tutor-assessment-container.component.html (1 hunks)
- src/main/webapp/app/exercises/shared/feedback/feedback.service.ts (2 hunks)
- src/main/webapp/app/exercises/shared/unreferenced-feedback/unreferenced-feedback.component.html (2 hunks)
- src/main/webapp/app/exercises/shared/unreferenced-feedback/unreferenced-feedback.component.ts (1 hunks)
- src/main/webapp/app/exercises/text/assess/text-submission-assessment.component.html (2 hunks)
- src/main/webapp/app/exercises/text/manage/example-text-submission/example-text-submission.component.html (1 hunks)
- src/test/javascript/spec/component/assessment-shared/unreferenced-feedback-detail.component.spec.ts (2 hunks)
- src/test/javascript/spec/service/feedback/feedback-service.spec.ts (2 hunks)
🧰 Additional context used
📓 Path-based instructions (17)
src/main/java/de/tum/cit/aet/artemis/assessment/service/AssessmentService.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/java/de/tum/cit/aet/artemis/assessment/service/ResultService.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/java/de/tum/cit/aet/artemis/programming/service/ProgrammingAssessmentService.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/java/de/tum/cit/aet/artemis/text/service/TextAssessmentService.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/assessment/unreferenced-feedback-detail/unreferenced-feedback-detail.component.ts (1)
src/main/webapp/app/course/learning-paths/services/base-api-http.service.ts (1)
src/main/webapp/app/exercises/file-upload/assess/file-upload-assessment.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/modeling/assess/modeling-assessment-editor/modeling-assessment-editor.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/modeling/manage/example-modeling/example-modeling-submission.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/programming/assess/code-editor-tutor-assessment-container.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/shared/feedback/feedback.service.ts (1)
src/main/webapp/app/exercises/shared/unreferenced-feedback/unreferenced-feedback.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/shared/unreferenced-feedback/unreferenced-feedback.component.ts (1)
src/main/webapp/app/exercises/text/assess/text-submission-assessment.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/text/manage/example-text-submission/example-text-submission.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/test/javascript/spec/component/assessment-shared/unreferenced-feedback-detail.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}}src/test/javascript/spec/service/feedback/feedback-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}}
🔇 Additional comments (32)
src/main/webapp/app/exercises/shared/feedback/feedback.service.ts (2)
3-3: LGTM: Service extension properly implementedThe extension of
BaseApiHttpServicealigns with the PR objectives to improve HTTP handling for long feedback text responses.Also applies to: 6-6
20-23: Verify integration with UnreferencedFeedbackDetailComponentThe service is used by UnreferencedFeedbackDetailComponent. Let's verify the integration is properly implemented.
✅ Verification successful
Let me gather more information about the actual integration by checking the implementation of UnreferencedFeedbackDetailComponent and FeedbackTextComponent.
Integration with UnreferencedFeedbackDetailComponent is properly implemented
The verification confirms:
- UnreferencedFeedbackDetailComponent correctly uses
getLongFeedbackTextto fetch and update feedback details- The service is properly tested with correct endpoint and response handling
- The integration follows the expected pattern where:
- The service method is called with required parameters (resultId, feedbackId)
- The response updates the feedback's detailText
- Changes are propagated via the onFeedbackChange event emitter
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of getLongFeedbackText in components ast-grep --pattern 'getLongFeedbackText($$$)' # Search for FeedbackService injection in components rg -l "FeedbackService" --type tsLength of output: 816
Script:
#!/bin/bash # Check implementation of UnreferencedFeedbackDetailComponent rg -A 10 "getLongFeedbackText" src/main/webapp/app/assessment/unreferenced-feedback-detail/unreferenced-feedback-detail.component.ts # Check implementation of FeedbackTextComponent rg -A 10 "getLongFeedbackText" src/main/webapp/app/exercises/shared/feedback/text/feedback-text.component.ts # Check the test implementation rg -A 10 "getLongFeedbackText" src/test/javascript/spec/service/feedback/feedback-service.spec.tsLength of output: 1135
src/test/javascript/spec/service/feedback/feedback-service.spec.ts (2)
1-3: LGTM! Proper setup of Angular testing utilities.The imports and test setup follow Angular testing best practices by using TestBed and HttpTestingController.
Also applies to: 9-9
35-47: Verify test coverage for all feedback scenarios.The test covers the basic functionality, but consider adding tests for:
- Very long feedback text (edge cases)
- Special characters in feedback
- Empty feedback handling
src/main/webapp/app/exercises/shared/unreferenced-feedback/unreferenced-feedback.component.html (3)
Line range hint
9-13: LGTM! Proper usage of new Angular syntax.The conditional rendering using
@iffollows the new Angular syntax guidelines correctly.
Line range hint
30-43: LGTM! Consistent implementation across feedback types.The feedback suggestions section maintains consistency with the unreferenced feedback section, properly implementing:
- New Angular
@forsyntaxresultIdbinding- Track expression for performance optimization
Line range hint
14-29: LGTM! Proper implementation of feedback iteration.The changes follow best practices:
- Uses new
@forsyntax correctly- Includes track expression for performance
- Adds
resultIdbinding which aligns with the PR objective of fixing long feedback displayLet's verify the
resultIdproperty is properly defined in the component:✅ Verification successful
The
resultIdproperty is properly defined and used correctlyThe component class has the
resultIdproperty properly defined as an input property:@Input() resultId: number;This matches with the template usage and confirms the property is correctly implemented.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify resultId property in the component class ast-grep --pattern 'class UnreferencedFeedbackComponent { $$$ resultId: $_ = $_ $$$ }'Length of output: 218
Script:
#!/bin/bash # Let's try a different approach to find the component file and check resultId fd "unreferenced-feedback.component.ts" --type f --exec cat {}Length of output: 6057
src/main/webapp/app/assessment/unreferenced-feedback-detail/unreferenced-feedback-detail.component.ts (2)
1-13: LGTM! Imports and component metadata are well-organized.The imports and component decorator follow Angular style guidelines correctly.
Line range hint
1-85: Verify the integration with manual assessment workflow.The implementation looks solid for handling long feedback. Let's verify the integration with the manual assessment workflow.
✅ Verification successful
Let me generate additional verification scripts to check the integration with the manual assessment workflow more thoroughly.
This will help us verify:
- How the unreferenced-feedback component (parent) handles the resultId
- The proper integration between parent and child components
- The complete flow of the manual assessment workflow
Based on the verification results, I can now provide the final response:
Integration with manual assessment workflow is properly implemented
The implementation shows correct integration with the manual assessment workflow through:
- Proper resultId propagation from parent
UnreferencedFeedbackComponentto childUnreferencedFeedbackDetailComponent- Consistent usage across various assessment templates (text, modeling, file-upload exercises)
- Correct handling of feedback changes and long feedback loading
The component is used consistently across the application with proper resultId binding, and the implementation aligns with the PR objectives for handling long feedback.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the component's integration in manual assessment templates # and ensure proper binding of resultId. echo "Checking manual assessment templates for component usage..." rg -l "jhi-unreferenced-feedback-detail" --type html echo "\nVerifying resultId binding in templates..." rg "\[resultId\]" --type html -A 2 -B 2Length of output: 8822
Script:
#!/bin/bash # Check the unreferenced-feedback component implementation echo "Checking unreferenced-feedback component implementation..." cat src/main/webapp/app/exercises/shared/unreferenced-feedback/unreferenced-feedback.component.ts echo "\nChecking unreferenced-feedback component template..." cat src/main/webapp/app/exercises/shared/unreferenced-feedback/unreferenced-feedback.component.html echo "\nChecking for resultId usage in the parent component..." rg "resultId.*: number" --type typescript -A 2 -B 2 src/main/webapp/app/exercises/shared/unreferenced-feedback/unreferenced-feedback.component.tsLength of output: 9024
src/test/javascript/spec/component/assessment-shared/unreferenced-feedback-detail.component.spec.ts (2)
18-18: LGTM: Import statement is correctly added.
40-40: Verify mock implementation for long feedback functionality.While the MockProvider is correctly configured, given that this PR implements long feedback functionality, we should verify if specific methods need to be mocked in the FeedbackService.
Let's check what methods we might need to mock:
✅ Verification successful
Mock implementation is sufficient for the test cases
The current mock implementation using
MockProvider(FeedbackService)is adequate because:
- The component only uses
getLongFeedbackTextmethod from FeedbackService- The test cases focus on feedback type changes, SGI updates, and deletion events
- None of the test cases explicitly verify the long feedback loading functionality, which is acceptable as it's an implementation detail
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for FeedbackService method calls in the component ast-grep --pattern 'FeedbackService.$_($$$)' # Search for long feedback related methods in FeedbackService rg -t typescript "long.*feedback" --glob "feedback.service.ts"Length of output: 268
Script:
#!/bin/bash # Search for FeedbackService method calls in the test file rg -A 3 "feedbackService" src/test/javascript/spec/component/assessment-shared/unreferenced-feedback-detail.component.spec.ts # Search for FeedbackService implementation fd "feedback.service.ts" --exec cat {}Length of output: 1309
Script:
#!/bin/bash # Search for component implementation to see how the service is used fd "unreferenced-feedback-detail.component.ts" --exec cat {} # Search for test cases in the spec file cat src/test/javascript/spec/component/assessment-shared/unreferenced-feedback-detail.component.spec.tsLength of output: 8398
src/main/java/de/tum/cit/aet/artemis/text/service/TextAssessmentService.java (3)
19-19: LGTM: Clean import addition.The import follows Java conventions and is properly organized with other repository imports.
45-47: LGTM: Clean constructor dependency injection.The constructor changes properly integrate the new repository:
- Follows constructor injection pattern
- Maintains consistent parameter ordering
- Correctly propagates dependency to superclass
Line range hint
63-94: Verify integration with long feedback handling.While the constructor changes support long feedback handling, the
prepareSubmissionForAssessmentmethod might need enhancement to properly handle long feedback loading, especially when loading existing results.Let's verify the feedback loading implementation in related services:
src/main/webapp/app/exercises/file-upload/assess/file-upload-assessment.component.html (2)
52-60: LGTM! Proper null checking and new input property added.The changes correctly implement:
- Conditional rendering based on result existence
- New
resultIdinput property for feedback persistence- Modern Angular
@ifsyntax as per guidelines
102-110: LGTM! Consistent implementation in assessment template.The implementation matches the main layout, ensuring consistent behavior across both locations.
Let's verify that this pattern is consistently applied across other assessment components:
✅ Verification successful
Consistent implementation pattern confirmed across assessment components
The implementation is indeed consistent across all assessment components in the codebase. All instances of
jhi-unreferenced-feedbackfollow the same pattern:
- Conditional rendering with
@if (result && result.id)- Common props:
feedbacks,readOnly,highlightDifferences,resultId- Consistent feedback change handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistent implementation of unreferenced-feedback component # across assessment templates # Test: Look for similar patterns in other assessment components rg -l "jhi-unreferenced-feedback" "src/main/webapp/app/exercises" | \ xargs rg -A 5 "@if.*result.*id.*jhi-unreferenced-feedback"Length of output: 124
Script:
#!/bin/bash # Let's try a different approach to find unreferenced-feedback usage patterns # First, find all files containing the component rg -l "jhi-unreferenced-feedback" "src/main/webapp/app/exercises" # Then let's examine the context around each usage echo "=== Detailed usage patterns ===" rg -B 2 -A 5 "jhi-unreferenced-feedback" "src/main/webapp/app/exercises"Length of output: 15096
src/main/webapp/app/exercises/shared/unreferenced-feedback/unreferenced-feedback.component.ts (1)
Line range hint
5-13: Component structure follows Angular best practices.The component is well-structured with proper separation of concerns, clear event handling, and focused methods.
src/main/webapp/app/exercises/programming/assess/code-editor-tutor-assessment-container.component.html (2)
Line range hint
1-1: Great job using the new Angular control flow syntax!The template consistently uses the new
@ifsyntax instead of the older*ngIf, which aligns with the current coding guidelines.Also applies to: 4-4, 13-13, 18-18, 67-67, 134-134, 141-141, 166-166
141-152: Feedback component changes look good and align with PR objectives.The conditional rendering of
jhi-unreferenced-feedbacknow properly checks for bothmanualResultandmanualResult.id, ensuring the component only renders when there's a valid result. The addition of[resultId]="manualResult.id"enables proper loading of long feedback text.This change directly addresses the issue of long feedback not being displayed by:
- Ensuring the feedback component only renders with a valid result ID
- Providing the result ID needed to fetch the complete feedback text
src/main/webapp/app/course/learning-paths/services/base-api-http.service.ts (2)
13-13: LGTM! Documentation improvement.The rewording makes the documentation more consistent with typical JSDoc style.
55-55: LGTM! Enhanced response handling.The changes improve the service's ability to handle text responses while simplifying the response handling logic. This aligns well with the PR's objective of fixing long feedback display issues.
Let's verify that all services extending BaseApiHttpService handle the response type changes correctly:
Also applies to: 60-63, 66-66
✅ Verification successful
Response type changes are correctly handled across the codebase
The verification confirms that:
- All services extending
BaseApiHttpServicehandle the response type changes correctly- The
responseType: 'text'is used appropriately inFeedbackService.getLongFeedbackText()which is the primary use case for this change- Other services extending
BaseApiHttpService(LearningPathApiService, FeedbackAnalysisService, CourseCompetencyApiService) use the default 'json' response type, which is compatible with the changesThe changes to the base service are safe and well-integrated across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for classes extending BaseApiHttpService to ensure they handle both response types ast-grep --pattern 'class $_ extends BaseApiHttpService { $$$ }' # Search for explicit responseType usage to ensure compatibility rg "responseType: ['|\"]text['|\"]"Length of output: 24234
src/main/webapp/app/exercises/modeling/assess/modeling-assessment-editor/modeling-assessment-editor.component.html (2)
67-78: Well-structured implementation with proper conditional rendering!The changes follow best practices by:
- Using the new
@ifsyntax as per guidelines- Adding proper null checks for
resultandresult.id- Including the
resultIdinput property for better feedback handling
67-78: Verify handling of long feedback in UnreferencedFeedbackComponent.While the template changes look good, let's verify the actual handling of long feedback in the component.
Also applies to: 150-158
✅ Verification successful
Let me generate additional scripts to check the UnreferencedFeedbackComponent implementation specifically:
This will help us verify how the component actually handles feedback display and if there are any length-related constraints.
Let me check one more aspect - the feedback collapse component since it appears to handle the actual feedback display:
This will help us understand how feedback text is actually handled and if there are any length limitations in place.
Long feedback is properly handled with collapsible display
The codebase has robust handling for long feedback through the FeedbackCollapseComponent which:
- Automatically collapses feedback longer than 300 characters
- Provides expand/collapse functionality for viewing full content
- Handles multi-line feedback appropriately
The changes in the template are safe and work well with the existing feedback display system.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check UnreferencedFeedbackComponent implementation for long feedback handling # Look for the component implementation echo "Searching for UnreferencedFeedbackComponent implementation..." rg -l "UnreferencedFeedbackComponent" --type ts # Check for any length limitations or truncation in the component echo "Checking for potential text truncation..." rg -i "truncate|maxLength|substring|slice" --type tsLength of output: 51452
Script:
#!/bin/bash # Check UnreferencedFeedbackComponent implementation and its feedback handling echo "Checking UnreferencedFeedbackComponent implementation..." rg -A 10 "export class UnreferencedFeedbackComponent" --type ts echo "Checking feedback-related templates and components..." rg -A 5 "class=\"feedback" src/main/webapp/app/exercises/shared/unreferenced-feedback/unreferenced-feedback.component.html || trueLength of output: 1847
Script:
#!/bin/bash # Check feedback collapse component implementation echo "Checking FeedbackCollapseComponent implementation..." rg -A 15 "export class FeedbackCollapseComponent" --type ts echo "Checking feedback collapse template..." rg -A 10 "feedback-collapse" --type html # Check for any feedback display constants echo "Checking for feedback-related constants..." rg "FEEDBACK_.*_LIMIT|FEEDBACK_.*_LENGTH" --type tsLength of output: 6732
src/main/webapp/app/exercises/text/manage/example-text-submission/example-text-submission.component.html (1)
162-170: LGTM! The changes align with PR objectives and coding guidelines.The conditional rendering of
<jhi-unreferenced-feedback>withresult.idcheck helps prevent potential issues with undefined feedback references, which aligns with fixing the long feedback display issues. The code also correctly uses the new@ifsyntax as per guidelines.Let's verify the component's usage across other assessment templates:
✅ Verification successful
Consistent usage of
jhi-unreferenced-feedbackwith proper result.id checks verified across all templatesThe verification shows that all assessment templates consistently implement the same pattern:
- All instances of
<jhi-unreferenced-feedback>are wrapped in@if (result && result.id)checks- This pattern is uniformly applied across different exercise types (text, programming, modeling, file-upload)
- The component usage follows the same structure with similar input bindings
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistent usage of jhi-unreferenced-feedback across assessment templates # Expected: Similar conditional rendering with result.id check # Search for jhi-unreferenced-feedback usage in assessment templates rg -g '*.html' -B 2 -A 2 '<jhi-unreferenced-feedback'Length of output: 8110
src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingAssessmentService.java (3)
6-6: LGTM: Import changes are clean and follow guidelines.The new imports are properly organized and avoid wildcards as per coding guidelines.
Also applies to: 20-20
53-56: LGTM: Constructor changes follow dependency injection best practices.The constructor properly implements constructor injection and correctly delegates to the superclass.
94-97: Verify complete feedback persistence flow.Let's ensure the feedback persistence changes are properly integrated across the system.
✅ Verification successful
Based on the shell script results, I can now generate the final response as there is sufficient evidence to verify the feedback persistence flow.
Feedback persistence flow is properly integrated
The changes are correctly integrated within the system's feedback persistence flow:
deleteLongFeedbackis consistently used across services before updating feedback itemsupdateAllFeedbackItemsis properly implemented in Result domain and used by all assessment services- Test coverage confirms proper handling of long feedback texts and manual assessments
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the complete feedback persistence flow # Check for other services using LongFeedbackTextRepository echo "Checking for other services using LongFeedbackTextRepository..." rg "LongFeedbackTextRepository" --type java # Check for related feedback persistence methods echo "Checking for related feedback persistence methods..." rg "deleteLongFeedback|updateAllFeedbackItems" --type java -A 5 # Look for potential feedback-related tests echo "Checking for related test coverage..." fd -e java -e kt --full-path ".*[Tt]est.*" | xargs rg "LongFeedbackText|ManualAssessment"Length of output: 19395
src/main/webapp/app/exercises/modeling/manage/example-modeling/example-modeling-submission.component.html (2)
Line range hint
1-1: LGTM: Consistent usage of new Angular control flow syntax.The template correctly uses the new
@ifand@forsyntax throughout, following the coding guidelines to prefer these over the older*ngIfand*ngFordirectives.Also applies to: 37-37, 42-42, 51-51, 67-67, 77-77, 89-89, 102-102, 111-111, 134-134, 141-141, 149-149, 156-156, 166-166, 186-186, 194-194
186-194: LGTM: Improved feedback component rendering logic.The changes enhance the feedback display by:
- Adding proper validation to ensure the component only renders with a valid result ID
- Providing the result ID to the feedback component for proper association
This aligns well with the PR's objective of fixing long feedback display issues.
src/main/java/de/tum/cit/aet/artemis/assessment/service/AssessmentService.java (2)
20-20: LGTM: Dependency injection follows best practices.The addition of
LongFeedbackTextRepositoryfollows the constructor injection pattern and maintains immutability with thefinalmodifier.Also applies to: 71-72, 76-76, 90-90
290-292: Consider adding error handling and transaction management.While the deletion of long feedback before saving prevents duplicate entries, there are a few concerns:
- The operation lacks explicit error handling for the
deleteLongFeedbackcall- Multiple database operations (delete + save) should be wrapped in a transaction to maintain data consistency
Let's verify the transaction management in the codebase:
Consider these improvements:
- Add error handling for the
deleteLongFeedbackoperation- Ensure proper transaction boundaries, either through:
- Adding
@Transactionalannotation to the method- Explicitly handling transactions in the service layer
src/main/java/de/tum/cit/aet/artemis/assessment/service/ResultService.java (1)
633-653: Use Batch Deletion for Efficiency indeleteLongFeedbackTo improve performance when deleting multiple
LongFeedbackTextentities, consider using batch deletion provided by the repository.[performance]
Modify the method to collect all IDs and perform a bulk delete:
List<Long> feedbackIds = feedbackList.stream() .filter(feedback -> feedback.getHasLongFeedbackText() && feedback.getId() != null) .map(Feedback::getId) .toList(); longFeedbackTextRepository.deleteByFeedbackIds(feedbackIds);Ensure that
deleteByFeedbackIdsis implemented inLongFeedbackTextRepositoryusing a suitable query for batch deletion.
src/test/javascript/spec/service/feedback/feedback-service.spec.ts
Outdated
Show resolved
Hide resolved
...webapp/app/assessment/unreferenced-feedback-detail/unreferenced-feedback-detail.component.ts
Show resolved
Hide resolved
...webapp/app/assessment/unreferenced-feedback-detail/unreferenced-feedback-detail.component.ts
Show resolved
Hide resolved
src/main/webapp/app/exercises/text/assess/text-submission-assessment.component.html
Outdated
Show resolved
Hide resolved
src/main/webapp/app/exercises/text/assess/text-submission-assessment.component.html
Show resolved
Hide resolved
...ercises/modeling/assess/modeling-assessment-editor/modeling-assessment-editor.component.html
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/assessment/service/ResultService.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/assessment/service/ResultService.java
Show resolved
Hide resolved
az108
temporarily deployed
to
artemis-test5.artemis.cit.tum.de
GitHub Actions
Inactive
az108
dismissed stale reviews from Malekos74, florian-glombik, and coderabbitai[bot]
via
e5dfc35
SindiBuklaji
temporarily deployed
to
artemis-test5.artemis.cit.tum.de
GitHub Actions
Inactive
krusche
changed the title
Programming exercises: Fix Long Manual Feedback not beeing displayedProgramming exercises: Fix an issue in which long manual feedback is not correctly displayed
Checklist
General
Server
Client
authoritiesto all new routes and checked the course groups for displaying navigation elements (links, buttons).Changes affecting Programming Exercises
Motivation and Context
Fixes #9434
This is a fix which adresses following issue where long feedback was not beeing loaded properly from the server.
This is most probably either caused by the result beeing damaged or not saved properly or the feedback beeing overwritten. To tackle the second mentioned potential reason following was done (described in Description).
Additionally another bug was fixed where instructors were not able to edit long feedback in manual assessements.
Description
I refactored the method which handles saving the long feedback when its created and when a long feedback is edited then the whole feedback is loaded to avoid that instructors override the long feedback with the short feedback which is loaded per default. The BaseAPIHTTPService on the client side was adjusted to also accept text responses from the server, making the rest call look very easy to understandable.
Note: The saving of manual assesements for programming exercise needs further refactoring but this is not part of this PR!
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
Performance Tests
Screenshots
Before:
After:
Summary by CodeRabbit
Release Notes
New Features
FeedbackServiceto fetch long feedback text dynamically.Bug Fixes
Documentation
BaseApiHttpService.Tests
FeedbackServicewith Angular's testing utilities and HTTP request mocking.