Development: Improve efficiency for deleting long manual feedback
#9652
Conversation
github-actions
bot
added
server
WalkthroughThe 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)
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
🧹 Outside diff range and nitpick comments (1)
src/main/java/de/tum/cit/aet/artemis/assessment/service/ResultService.java (1)
Line range hint
634-656: Consider optimizing the feedback list handling.The method creates a new ArrayList unnecessarily since the list is only used once and immediately passed to
updateAllFeedbackItems. Consider passing the feedbackList directly if immutability isn't required.public void deleteLongFeedback(List<Feedback> feedbackList, Result result) { if (feedbackList == null) { return; } List<Long> feedbackIdsWithLongText = feedbackList.stream() .filter(feedback -> feedback.getHasLongFeedbackText() && feedback.getId() != null) .map(Feedback::getId) .toList(); longFeedbackTextRepository.deleteByFeedbackIds(feedbackIdsWithLongText); - List<Feedback> feedbacks = new ArrayList<>(feedbackList); - result.updateAllFeedbackItems(feedbacks, true); + result.updateAllFeedbackItems(feedbackList, true); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
src/main/java/de/tum/cit/aet/artemis/assessment/service/ResultService.java(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/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_imports
🔇 Additional comments (1)
src/main/java/de/tum/cit/aet/artemis/assessment/service/ResultService.java (1)
655-655: Verify the impact of enabling automatic assessment.
The change to pass true to updateAllFeedbackItems indicates that feedback updates will be treated as automatic assessments. While this may improve efficiency by bypassing test validations, ensure this doesn't bypass important validation checks or affect the feedback quality.
✅ Verification successful
Looking at the output, I can now see the implementation of updateAllFeedbackItems in Result.java and its usage across the codebase. I can generate the final response as we have enough context to understand the impact of the skipAutomaticResults parameter (previously named incorrectly as automatic in my search).
Setting skipAutomaticResults=true is correct for feedback deletion
The implementation in Result.java shows that skipAutomaticResults=true causes the method to skip processing automatic feedback items (feedback.getType() == FeedbackType.AUTOMATIC). This is the correct behavior for the deleteLongFeedback method since:
- It's only meant to handle manual feedback deletion
- Other services appropriately use
falsewhen they need to process all feedback types - The change aligns with the PR's goal of making manual feedback deletion more efficient
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential validation bypasses in updateAllFeedbackItems
# Look for validation logic in the method that might be skipped
ast-grep --pattern 'updateAllFeedbackItems($_, $automatic) {
$$$
if (!$automatic) {
$$$
}
$$$
}'
Length of output: 234
Script:
#!/bin/bash
# Let's try to find the Result class and check the updateAllFeedbackItems method implementation
rg -l "class Result" --type java
# Then let's examine the updateAllFeedbackItems method
rg "updateAllFeedbackItems" --type java -A 10
Length of output: 5329
github-actions
bot
added
tests
modeling
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/test/java/de/tum/cit/aet/artemis/modeling/ModelingAssessmentIntegrationTest.java (1)
Line range hint
1674-1676: Avoid direct database access in tests; use service methods or mocks instead.In the test method
testdeleteResult, direct access tosubmissionRepositoryis used. According to the coding guidelines (avoid_db_access: true), tests should avoid direct database access and utilize service methods or mocks.Consider modifying the code as follows:
- submission = submissionRepository.findOneWithEagerResultAndFeedbackAndAssessmentNote(submission.getId()); + submission = submissionService.findOneWithEagerResultAndFeedbackAndAssessmentNote(submission.getId());Ensure that you use the appropriate service method to interact with the submission data.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
src/main/java/de/tum/cit/aet/artemis/assessment/service/ResultService.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/modeling/ModelingAssessmentIntegrationTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/de/tum/cit/aet/artemis/assessment/service/ResultService.java
🧰 Additional context used
📓 Path-based instructions (1)
src/test/java/de/tum/cit/aet/artemis/modeling/ModelingAssessmentIntegrationTest.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/modeling/ModelingAssessmentIntegrationTest.java (3)
Line range hint 1017-1050: Test method testOverrideAutomaticAssessment follows coding guidelines and correctly tests the override functionality.
Line range hint 1176-1210: Test method testOverrideSubmittedManualAssessment_noConflict is well-structured and adheres to guidelines.
Line range hint 1654-1666: Test method testCheckPlagiarismIdenticalLongModels effectively tests plagiarism detection with identical models.
HawKhiem
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
GitHub Actions
Inactive
There was a problem hiding this comment.
[Question] I want to make sure I understand the changes before this is merged, therefore marked as request changes:
Why does deleteLongFeedback now only update manual results? There can be long feedback from automatic tests. Why does this not break the result score in such cases?
I would consider me quite familiar with how programming exercises and the long feedback work in Artemis. Since the answer is not obvious to me, I would expect it is even less so for someone else in the future. Please add an inline comment in the code why we can ignore automatic results here.
|
@b-fein The update is only necessary to reload the feedback from the database when feedback is of type “lazy fetch.” The reason we can ignore automatic results for long feedback is that, based on my analysis, this error only occurred due to incorrectly handled manual submissions. In other words, this fix is required only for manual assessments, not for automatic assessments. The method typically includes a comment stating that it should not be used for fully automatic assessments. Would you like me to add another inline comment? |
Ah, I only now have seen that the |
|
@b-fein I verified that the current method is used exclusively for manual assessments. If anyone wants to use the deleteLongFeedback method in the future, they would naturally review its content and see that the updateFeedback method already includes an inline comment advising against its use for fully automatic results. However, if you still prefer, I can add another comment. I felt it was unnecessary, which is why I initially left it out. |
To be honest, I don’t think they would. I would expect people to read at best the Javadoc. Since neither the method name nor the Javadoc mention any kind of limitation, that is the point where they will decide ‘Sounds good, I’ll use it’. And even if I’m assuming the worst and people look at the code, I think it’s unlikely that they will check every called method, too. So, yes, I definitely would prefer a clear description of the precondition right after the first sentence of the Javadoc. |
az108
dismissed stale reviews from JohannesStoehr, florian-glombik, HawKhiem, Jan-Thurner, JohannesWt, and coderabbitai[bot]
via
cbafc5b
krusche
changed the title
Programming exercises: Make deleting of long manual feedback more efficientDevelopment: Make deleting of long manual feedback more efficient
krusche
changed the title
Development: Make deleting of long manual feedback more efficientDevelopment: Improve efficiency for deleting long manual feedback
Checklist
General
Server
Changes affecting Programming Exercises
Motivation and Context
Follow up #9562
While going through more refactor possibilities, I noticed a slightly more efficient implementation.
Description
I changed the flag for automatic assesements to true, allowing the updateFeedback method to skip all automatic tests instead of iterating over them aswell.
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
Summary by CodeRabbit
New Features
Bug Fixes
Refactor