Lectures: Correctly put and get competencies for attachment units
#9684
Conversation
github-actions
bot
added
server
WalkthroughThe changes in this pull request focus on the Changes
Possibly related PRs
Suggested labels
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: 0
🧹 Outside diff range and nitpick comments (4)
src/main/java/de/tum/cit/aet/artemis/lecture/service/AttachmentUnitService.java (2)
120-120: Add validation and defensive copying for competency links.While the competency links update works, consider these improvements for better robustness:
- Validate the incoming competency links before assignment
- Use defensive copying to prevent external modifications
- existingAttachmentUnit.setCompetencyLinks(updateUnit.getCompetencyLinks()); + Set<CompetencyLectureUnitLink> newLinks = updateUnit.getCompetencyLinks(); + if (newLinks != null) { + existingAttachmentUnit.setCompetencyLinks(new HashSet<>(newLinks)); + } else { + existingAttachmentUnit.setCompetencyLinks(new HashSet<>()); + }
Line range hint
116-159: Consider refactoring for better separation of concerns.The
updateAttachmentUnitmethod handles multiple responsibilities (competency updates, file handling, attachment updates) making it complex and harder to maintain. Consider breaking it down into smaller, focused methods:
- Extract competency handling logic
- Extract file handling logic
- Extract attachment update logic
Example refactoring structure:
public AttachmentUnit updateAttachmentUnit(...) { // Main orchestration validatePreConditions(existingAttachmentUnit); AttachmentUnit savedUnit = updateBasicProperties(existingAttachmentUnit, updateUnit); updateCompetencies(savedUnit, updateUnit); updateAttachmentIfNeeded(savedUnit, updateAttachment); handleFileIfProvided(savedUnit, updateFile, keepFilename); return finalizeUpdate(savedUnit); } private void updateCompetencies(AttachmentUnit existingUnit, AttachmentUnit updateUnit) { Set<CompetencyLectureUnitLink> existingLinks = new HashSet<>(existingUnit.getCompetencyLinks()); // Update logic... competencyProgressService.updateProgressForUpdatedLearningObjectAsync(...); } private void handleFileIfProvided(...) { if (updateFile == null) return; // File handling logic... }This refactoring would:
- Improve code readability
- Make the code easier to test
- Make the code easier to maintain
- Reduce cognitive complexity
src/main/java/de/tum/cit/aet/artemis/lecture/web/AttachmentUnitResource.java (2)
103-103: Update JavaDoc to reflect competency data in response.The method now returns competency data along with the attachment unit, but this isn't documented in the JavaDoc. Consider updating the documentation to reflect this change.
- * @return the ResponseEntity with status 200 (OK) and with body the attachment unit, or with status 404 (Not Found) + * @return the ResponseEntity with status 200 (OK) and with body the attachment unit including slides and competencies, or with status 404 (Not Found)
103-103: Consider lazy loading optimization if competencies aren't always needed.The change from
findByIdElseThrowtofindOneWithSlidesAndCompetencieseagerly loads additional data. Consider implementing a separate endpoint or query parameter if this additional data isn't always required by clients.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
src/main/java/de/tum/cit/aet/artemis/lecture/service/AttachmentUnitService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/lecture/web/AttachmentUnitResource.java(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/main/java/de/tum/cit/aet/artemis/lecture/service/AttachmentUnitService.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
src/main/java/de/tum/cit/aet/artemis/lecture/web/AttachmentUnitResource.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/lecture/web/AttachmentUnitResource.java (1)
103-106: LGTM! The implementation aligns with PR objectives.
The change correctly implements the requirement to include competency data when retrieving attachment units. The implementation:
- Follows proper authorization checks
- Maintains error handling
- Uses appropriate repository method
WalkthroughThe changes in this pull request focus on enhancing the Changes
Possibly related PRs
Suggested labels
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
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/main/java/de/tum/cit/aet/artemis/lecture/service/AttachmentUnitService.java (2)
Line range hint
116-171: Consider breaking down the updateAttachmentUnit method.The method handles multiple concerns (competency updates, file handling, progress tracking) making it harder to maintain and test. Consider extracting the file handling logic into a separate method.
Example refactor:
public AttachmentUnit updateAttachmentUnit(...) { Set<CompetencyLectureUnitLink> existingCompetencyLinks = new HashSet<>(existingAttachmentUnit.getCompetencyLinks()); // Update basic properties updateBasicProperties(existingAttachmentUnit, updateUnit); AttachmentUnit savedAttachmentUnit = lectureUnitService.saveWithCompetencyLinks(existingAttachmentUnit, attachmentUnitRepository::saveAndFlush); // Handle attachment updates updateAttachmentAndFile(savedAttachmentUnit, updateAttachment, updateFile, keepFilename); // Handle competency progress updateCompetencyProgress(existingAttachmentUnit, updateUnit, existingCompetencyLinks); return savedAttachmentUnit; } +private void updateBasicProperties(AttachmentUnit existingUnit, AttachmentUnit updateUnit) { + existingUnit.setDescription(updateUnit.getDescription()); + existingUnit.setName(updateUnit.getName()); + existingUnit.setReleaseDate(updateUnit.getReleaseDate()); + existingUnit.setCompetencyLinks(updateUnit.getCompetencyLinks()); +} +private void updateAttachmentAndFile(AttachmentUnit unit, Attachment updateAttachment, MultipartFile file, boolean keepFilename) { + // Extract file handling logic here +} +private void updateCompetencyProgress(AttachmentUnit existingUnit, AttachmentUnit updateUnit, Set<CompetencyLectureUnitLink> originalLinks) { + existingUnit.setCompetencyLinks(originalLinks); + competencyProgressService.updateProgressForUpdatedLearningObjectAsync(existingUnit, Optional.of(updateUnit)); +}
Line range hint
116-171: Consider adding explicit transaction boundaries.The method performs multiple database operations without explicit transaction boundaries. Consider adding
@Transactionalto ensure atomic updates.+@Transactional public AttachmentUnit updateAttachmentUnit(AttachmentUnit existingAttachmentUnit, AttachmentUnit updateUnit, Attachment updateAttachment, MultipartFile updateFile, boolean keepFilename) { // ... existing code ... }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
src/main/java/de/tum/cit/aet/artemis/lecture/service/AttachmentUnitService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/lecture/web/AttachmentUnitResource.java(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/main/java/de/tum/cit/aet/artemis/lecture/service/AttachmentUnitService.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
src/main/java/de/tum/cit/aet/artemis/lecture/web/AttachmentUnitResource.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 (2)
src/main/java/de/tum/cit/aet/artemis/lecture/service/AttachmentUnitService.java (1)
120-120: LGTM: Competency links update implementation.
The implementation correctly updates the competency links while preserving the original links for progress tracking.
src/main/java/de/tum/cit/aet/artemis/lecture/web/AttachmentUnitResource.java (1)
103-103: Ensure efficient fetching of associated entities to prevent N+1 queries
By using findOneWithSlidesAndCompetencies, ensure that slides and competencies are fetched eagerly to avoid potential N+1 query problems and improve performance when accessing these associations.
Run the following script to verify the fetching strategy:
✅ Verification successful
Let me check the actual implementation of this method by looking for the query definition.
The query is already optimized with LEFT JOIN FETCH statements
The findOneWithSlidesAndCompetencies method is properly implemented with eager fetching using LEFT JOIN FETCH for slides, competency links, and competencies. This prevents N+1 query issues by fetching all related entities in a single query.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if 'findOneWithSlidesAndCompetencies' method fetches slides and competencies eagerly.
# Expected result: The method uses JOIN FETCH to eagerly load slides and competencies.
rg -A 5 'findOneWithSlidesAndCompetencies' src/main/java/de/tum/cit/aet/artemis/lecture/repository/AttachmentUnitRepository.java
Length of output: 233
Script:
#!/bin/bash
# Look for @Query annotation above the findOneWithSlidesAndCompetencies method
rg -B 5 "findOneWithSlidesAndCompetencies" src/main/java/de/tum/cit/aet/artemis/lecture/repository/AttachmentUnitRepository.java
# Also check if there's any named query definition in XML or properties files
fd -e xml -e properties | xargs rg "findOneWithSlidesAndCompetencies"
Length of output: 547
|
Welcome to Artemis 👋 Can you also add some server tests to check for this behaviour? |
There was a problem hiding this comment.
@Strohgelaender is right - please add a small test for this so it doesn't happen again :)
bassner
changed the title
bassner
changed the title
bassner
changed the title
Lectures: Correctly put and get competencies for attachment units
|
Thank you for your help with fixing the issue. Since we want to fix the bug quickly, I adapted the tests in a new PR and incorporated your changes in this PR: #9739 Feel free to assign yourself as well in this PR. As our readme states (https://github.com/ls1intum/Artemis#communication), we also use Slack for communication. If you want to contribute more in the future, feel free to sign up so we can better communicate and easier collaborate on PRs in the future. |
Thanks for picking this up - as I have been very busy this past week I havn't had time to add the tests. I will definitely join the slack, thanks for the hint. |
|
Closed in favor of #9739 |
Checklist
General
Server
Motivation and Context
Fixes this issue: #9666
Description
attachment-unitsGET-Requestattachment-unitsPUT-Request (Updates the attachment unit)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
Exam Mode Test
Performance Tests
Screenshots
Summary by CodeRabbit
New Features
Bug Fixes
Documentation