Adaptive learning: Fix linking attachment units to competencies
#9739
Conversation
github-actions
bot
added
tests
server
WalkthroughThe changes in this pull request involve significant modifications to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 pmdsrc/main/java/de/tum/cit/aet/artemis/lecture/repository/AttachmentUnitRepository.javaThe following rules are missing or misspelled in your ruleset file category/vm/bestpractices.xml: BooleanInstantiation, DontImportJavaLang, DuplicateImports, EmptyFinallyBlock, EmptyIfStmt, EmptyInitializer, EmptyStatementBlock, EmptyStatementNotInLoop, EmptySwitchStatements, EmptySynchronizedBlock, EmptyTryBlock, EmptyWhileStmt, ExcessiveClassLength, ExcessiveMethodLength, ImportFromSamePackage, MissingBreakInSwitch, SimplifyBooleanAssertion. Please check your ruleset configuration. src/main/java/de/tum/cit/aet/artemis/lecture/web/AttachmentUnitResource.javaThe following rules are missing or misspelled in your ruleset file category/vm/bestpractices.xml: BooleanInstantiation, DontImportJavaLang, DuplicateImports, EmptyFinallyBlock, EmptyIfStmt, EmptyInitializer, EmptyStatementBlock, EmptyStatementNotInLoop, EmptySwitchStatements, EmptySynchronizedBlock, EmptyTryBlock, EmptyWhileStmt, ExcessiveClassLength, ExcessiveMethodLength, ImportFromSamePackage, MissingBreakInSwitch, SimplifyBooleanAssertion. Please check your ruleset configuration. 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 (5)
src/main/java/de/tum/cit/aet/artemis/lecture/service/AttachmentUnitService.java (3)
Line range hint
119-157: Consider extracting file handling logic into a separate method.The file handling logic within
updateAttachmentUnitis quite complex and could benefit from being extracted into a dedicated method for better maintainability and readability.Consider applying this refactor:
public AttachmentUnit updateAttachmentUnit(AttachmentUnit existingAttachmentUnit, AttachmentUnit updateUnit, Attachment updateAttachment, MultipartFile updateFile, boolean keepFilename) { Set<CompetencyLectureUnitLink> existingCompetencyLinks = new HashSet<>(existingAttachmentUnit.getCompetencyLinks()); existingAttachmentUnit.setDescription(updateUnit.getDescription()); existingAttachmentUnit.setName(updateUnit.getName()); existingAttachmentUnit.setReleaseDate(updateUnit.getReleaseDate()); existingAttachmentUnit.setCompetencyLinks(updateUnit.getCompetencyLinks()); AttachmentUnit savedAttachmentUnit = lectureUnitService.saveWithCompetencyLinks(existingAttachmentUnit, attachmentUnitRepository::saveAndFlush); Attachment existingAttachment = existingAttachmentUnit.getAttachment(); if (existingAttachment == null) { throw new BadRequestAlertException("Attachment unit must be associated to an attachment", "AttachmentUnit", "attachmentMissing"); } updateAttachment(existingAttachment, updateAttachment, savedAttachmentUnit); handleFile(updateFile, existingAttachment, keepFilename, savedAttachmentUnit.getId()); final int revision = existingAttachment.getVersion() == null ? 1 : existingAttachment.getVersion() + 1; existingAttachment.setVersion(revision); Attachment savedAttachment = attachmentRepository.saveAndFlush(existingAttachment); savedAttachmentUnit.setAttachment(savedAttachment); prepareAttachmentUnitForClient(savedAttachmentUnit); evictCache(updateFile, savedAttachmentUnit); - if (updateFile != null) { - if (existingAttachmentUnit.getSlides() != null && !existingAttachmentUnit.getSlides().isEmpty()) { - List<Slide> slides = existingAttachmentUnit.getSlides(); - for (Slide slide : slides) { - fileService.schedulePathForDeletion(FilePathService.actualPathForPublicPathOrThrow(URI.create(slide.getSlideImagePath())), 5); - } - slideRepository.deleteAll(existingAttachmentUnit.getSlides()); - } - // Split the updated file into single slides only if it is a pdf - if (Objects.equals(FilenameUtils.getExtension(updateFile.getOriginalFilename()), "pdf")) { - slideSplitterService.splitAttachmentUnitIntoSingleSlides(savedAttachmentUnit); - } - if (pyrisWebhookService.isPresent() && irisSettingsRepository.isPresent()) { - pyrisWebhookService.get().autoUpdateAttachmentUnitsInPyris(savedAttachmentUnit.getLecture().getCourse().getId(), List.of(savedAttachmentUnit)); - } - } + handleFileUpdate(updateFile, existingAttachmentUnit, savedAttachmentUnit); // Set the original competencies back to the attachment unit so that the competencyProgressService can determine which competencies changed existingAttachmentUnit.setCompetencyLinks(existingCompetencyLinks); competencyProgressService.updateProgressForUpdatedLearningObjectAsync(existingAttachmentUnit, Optional.of(updateUnit)); return savedAttachmentUnit; } +/** + * Handles the update of file-related operations including slide management and Pyris integration. + * + * @param updateFile The file to update + * @param existingAttachmentUnit The existing attachment unit + * @param savedAttachmentUnit The saved attachment unit + */ +private void handleFileUpdate(MultipartFile updateFile, AttachmentUnit existingAttachmentUnit, AttachmentUnit savedAttachmentUnit) { + if (updateFile == null) { + return; + } + + if (existingAttachmentUnit.getSlides() != null && !existingAttachmentUnit.getSlides().isEmpty()) { + List<Slide> slides = existingAttachmentUnit.getSlides(); + for (Slide slide : slides) { + fileService.schedulePathForDeletion(FilePathService.actualPathForPublicPathOrThrow(URI.create(slide.getSlideImagePath())), 5); + } + slideRepository.deleteAll(existingAttachmentUnit.getSlides()); + } + + // Split the updated file into single slides only if it is a pdf + if (Objects.equals(FilenameUtils.getExtension(updateFile.getOriginalFilename()), "pdf")) { + slideSplitterService.splitAttachmentUnitIntoSingleSlides(savedAttachmentUnit); + } + + if (pyrisWebhookService.isPresent() && irisSettingsRepository.isPresent()) { + pyrisWebhookService.get().autoUpdateAttachmentUnitsInPyris(savedAttachmentUnit.getLecture().getCourse().getId(), List.of(savedAttachmentUnit)); + } +}
Line range hint
182-186: Fix duplicatesetReleaseDatecall inupdateAttachmentmethod.The
setReleaseDatemethod is called twice with the same parameter.Apply this fix:
private void updateAttachment(Attachment existingAttachment, Attachment updateAttachment, AttachmentUnit attachmentUnit) { // Make sure that the original references are preserved. existingAttachment.setAttachmentUnit(attachmentUnit); - existingAttachment.setReleaseDate(updateAttachment.getReleaseDate()); existingAttachment.setName(updateAttachment.getName()); existingAttachment.setReleaseDate(updateAttachment.getReleaseDate()); existingAttachment.setAttachmentType(updateAttachment.getAttachmentType()); }
Line range hint
195-203: Consider using Path.resolve() for path construction.The current implementation uses string concatenation for path construction. Using
Path.resolve()would be more idiomatic and safer.Apply this refactor:
private void handleFile(MultipartFile file, Attachment attachment, boolean keepFilename, Long attachmentUnitId) { if (file != null && !file.isEmpty()) { - Path basePath = FilePathService.getAttachmentUnitFilePath().resolve(attachmentUnitId.toString()); + Path basePath = FilePathService.getAttachmentUnitFilePath().resolve(String.valueOf(attachmentUnitId)); Path savePath = fileService.saveFile(file, basePath, keepFilename); attachment.setLink(FilePathService.publicPathForActualPathOrThrow(savePath, attachmentUnitId).toString()); attachment.setUploadDate(ZonedDateTime.now()); } }src/test/java/de/tum/cit/aet/artemis/lecture/AttachmentUnitIntegrationTest.java (2)
281-281: Consider enhancing assertion specificity for competency links.While the current assertion verifies the presence of the competency link, consider making it more specific by checking the complete set of competency links and their properties.
- assertThat(attachmentUnit1.getCompetencyLinks()).anyMatch(link -> link.getCompetency().getId().equals(competency.getId())); + assertThat(attachmentUnit1.getCompetencyLinks()) + .hasSize(1) + .element(0) + .satisfies(link -> { + assertThat(link.getCompetency().getId()).isEqualTo(competency.getId()); + assertThat(link.getWeight()).isEqualTo(1); + });
338-338: Enhance test readability and assertion coverage.While the test correctly verifies competency linking, consider:
- Adding comments to clearly separate test phases (Arrange-Act-Assert)
- Expanding assertions to verify all properties of the competency link
+ // Arrange: Setup competency link competencyUtilService.linkLectureUnitToCompetency(competency, attachmentUnit); // 1. check the database call directly this.attachmentUnit = this.attachmentUnitRepository.findByIdElseThrow(this.attachmentUnit.getId()); assertThat(this.attachmentUnit.getAttachment()).isEqualTo(this.attachment); // 2. check the REST call this.attachmentUnit = request.get("/api/lectures/" + lecture1.getId() + "/attachment-units/" + this.attachmentUnit.getId(), HttpStatus.OK, AttachmentUnit.class); assertThat(this.attachmentUnit.getAttachment()).isEqualTo(this.attachment); - assertThat(this.attachmentUnit.getCompetencyLinks()).anyMatch(link -> link.getCompetency().getId().equals(competency.getId())); + // Verify: Check competency link properties + assertThat(this.attachmentUnit.getCompetencyLinks()) + .hasSize(1) + .element(0) + .satisfies(link -> { + assertThat(link.getCompetency().getId()).isEqualTo(competency.getId()); + assertThat(link.getLectureUnit()).isEqualTo(this.attachmentUnit); + assertThat(link.getWeight()).isEqualTo(1); + });Also applies to: 347-347
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
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)src/test/java/de/tum/cit/aet/artemis/lecture/AttachmentUnitIntegrationTest.java(3 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
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
src/test/java/de/tum/cit/aet/artemis/lecture/AttachmentUnitIntegrationTest.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/main/java/de/tum/cit/aet/artemis/lecture/service/AttachmentUnitService.java (1)
120-120: LGTM: Competency links update is properly implemented.
The implementation correctly updates the competency links while maintaining the original links for progress tracking. The original links are preserved and later used by competencyProgressService to determine changes.
src/test/java/de/tum/cit/aet/artemis/lecture/AttachmentUnitIntegrationTest.java (1)
281-281: LGTM! Test changes effectively cover competency link functionality.
The added assertions properly verify the competency linking feature while maintaining good test practices. The suggestions above are optional improvements for better test maintainability.
Also applies to: 338-338, 347-347
src/main/java/de/tum/cit/aet/artemis/lecture/web/AttachmentUnitResource.java (1)
103-103: LGTM: Fetching slides and competencies as required
The use of findOneWithSlidesAndCompetencies ensures that the AttachmentUnit is retrieved along with its associated slides and competencies. This is appropriate if these associations are needed for the response or further processing.
sachmii
temporarily deployed
to
artemis-test6.artemis.cit.tum.de
GitHub Actions
Inactive
JohannesStoehr
dismissed stale reviews from sachmii and coderabbitai[bot]
via
54f04f4
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/repository/AttachmentUnitRepository.java (1)
68-70: Add JavaDoc documentation for the new method.While the implementation is correct, adding JavaDoc would improve clarity by documenting:
- Method purpose
- Parameter description
- Return value
- Exception scenarios
Example documentation:
/** * Retrieves an attachment unit with its slides and competencies by ID. * * @param attachmentUnitId the ID of the attachment unit to retrieve * @return the attachment unit with its associated slides and competencies * @throws EntityNotFoundException if no attachment unit is found with the given ID */ default AttachmentUnit findWithSlidesAndCompetenciesByIdElseThrow(long attachmentUnitId) { return getValueElseThrow(findWithSlidesAndCompetenciesById(attachmentUnitId), attachmentUnitId); }src/main/java/de/tum/cit/aet/artemis/lecture/web/AttachmentUnitResource.java (1)
Line range hint
128-137: LGTM! Consider enhancing error handling for file uploads.The change to use
findWithSlidesAndCompetenciesByIdElseThrowis consistent with the performance optimization pattern. However, the file upload handling could be improved.Consider wrapping the file handling in a try-catch block to provide more specific error messages:
AttachmentUnit existingAttachmentUnit = attachmentUnitRepository.findWithSlidesAndCompetenciesByIdElseThrow(attachmentUnitId); checkAttachmentUnitCourseAndLecture(existingAttachmentUnit, lectureId); authorizationCheckService.checkHasAtLeastRoleInCourseElseThrow(Role.EDITOR, existingAttachmentUnit.getLecture().getCourse(), null); - AttachmentUnit savedAttachmentUnit = attachmentUnitService.updateAttachmentUnit(existingAttachmentUnit, attachmentUnit, attachment, file, keepFilename); + AttachmentUnit savedAttachmentUnit; + try { + savedAttachmentUnit = attachmentUnitService.updateAttachmentUnit(existingAttachmentUnit, attachmentUnit, attachment, file, keepFilename); + } catch (IOException e) { + log.error("Failed to process file upload for attachment unit {}: {}", attachmentUnitId, e.getMessage()); + throw new InternalServerErrorException("Failed to process file upload"); + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
src/main/java/de/tum/cit/aet/artemis/lecture/repository/AttachmentUnitRepository.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/lecture/web/AttachmentUnitResource.java(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/main/java/de/tum/cit/aet/artemis/lecture/repository/AttachmentUnitRepository.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 (4)
src/main/java/de/tum/cit/aet/artemis/lecture/repository/AttachmentUnitRepository.java (3)
6-6: LGTM!
The Optional import is correctly added and follows Java import guidelines.
66-66: Well-structured query with proper relationship handling!
The implementation demonstrates several good practices:
- Proper use of LEFT JOIN FETCH for efficient loading of relationships
- Clear query structure that includes competency-related joins
- Return type change to Optional follows Java best practices for nullable results
66-66: Verify query performance with larger datasets.
The query joins multiple tables (attachmentUnit, slides, competencyLinks, competency). While the LEFT JOIN FETCH is appropriate for eager loading, we should verify performance with larger datasets.
✅ Verification successful
Let me gather more information about the query implementation and related entities.
Let me check the actual query implementation and entity relationships one more time.
Based on the results, I can now generate a final response as we have enough information about the entity relationships and query implementation.
LEFT JOIN FETCH is appropriate for this use case
The implementation is well-designed for the following reasons:
- The
AttachmentUnitentity uses lazy loading (fetch = FetchType.LAZY) for its relationships - The query is used for a single entity lookup by ID, not for bulk operations
- The fetch joins are necessary to avoid N+1 queries when accessing related entities
The current implementation follows JPA best practices for eager loading specific relationships in a single query. Performance impact would be minimal since:
- It's an ID-based lookup returning at most one record
- The JOIN operations are LEFT joins, appropriate for optional relationships
- The query is used in a focused context for loading a complete entity graph
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for potential N+1 queries or performance issues
rg -A 10 "AttachmentUnit.*findWithSlidesAndCompetenciesById" src/test/java/ || echo "No existing performance tests found"
# Look for similar query patterns in the codebase for consistency
ast-grep --pattern 'LEFT JOIN FETCH $_.competencyLinks'
Length of output: 216
Script:
#!/bin/bash
# Check the query implementation
rg -A 5 "@Query.*findWithSlidesAndCompetenciesById" src/
# Look for entity relationships and fetch types
ast-grep --pattern 'class AttachmentUnit {
$$$
}'
# Check for similar fetch patterns in other repositories
rg -l "LEFT JOIN FETCH" src/
Length of output: 7388
Script:
#!/bin/bash
# Check the actual query implementation in AttachmentUnitRepository
rg -B 2 -A 5 "findWithSlidesAndCompetenciesById" src/main/java/de/tum/cit/aet/artemis/lecture/repository/AttachmentUnitRepository.java
# Look for entity relationships in AttachmentUnit
rg -B 2 -A 10 "class AttachmentUnit" src/main/java/de/tum/cit/aet/artemis/lecture/
# Check if there are any existing performance tests for similar queries
rg "performance.*test.*LEFT JOIN FETCH" src/test/
Length of output: 5038
src/main/java/de/tum/cit/aet/artemis/lecture/web/AttachmentUnitResource.java (1)
103-103: LGTM! Performance optimization for competency links.
The change to use findWithSlidesAndCompetenciesByIdElseThrow improves performance by eager loading slides and competencies in a single query.
Let's verify the repository method implementation:
✅ Verification successful
Repository method verified and properly implemented
The findWithSlidesAndCompetenciesByIdElseThrow method is correctly implemented in the repository:
- It's a default method that wraps
findWithSlidesAndCompetenciesById - Returns an
AttachmentUnitwith proper error handling viagetValueElseThrow - The base method returns
Optional<AttachmentUnit>as expected for JPA repositories
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the repository method implementation
# Expected: Method should exist in AttachmentUnitRepository with proper JPA query
ast-grep --pattern 'interface AttachmentUnitRepository {
$$$
$_ findWithSlidesAndCompetenciesByIdElseThrow($_) {
$$$
}
$$$
}'
Length of output: 263
Script:
#!/bin/bash
# Let's try a different approach to find the repository interface and method
# First, find the repository file
fd "AttachmentUnitRepository.java" --type f
# Then let's use ripgrep to search for the method as interfaces don't have method bodies
rg "findWithSlidesAndCompetenciesByIdElseThrow" -A 2
Length of output: 1800
Script:
#!/bin/bash
# Let's check the findWithSlidesAndCompetenciesById method to understand the full implementation
rg "findWithSlidesAndCompetenciesById\(" -A 5 src/main/java/de/tum/cit/aet/artemis/lecture/repository/AttachmentUnitRepository.java
Length of output: 456
BBesrour
temporarily deployed
to
artemis-test2.artemis.cit.tum.de
GitHub Actions
Inactive
BBesrour
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
There was a problem hiding this comment.
Tested on TS3.
Not sure If I did the steps correctly but the issue still persists for me on Firefox.
I created this unit
Then linked a competency (competency was not newly created)
Competency still not linked when clicking on edit again
It is linked if I go back and click on edit again or if I refresh the page
That's ok, we only fix the persistence on the server. This PR is not about the returned object to the client |
MaximilianAnzinger
added
the
maintainer-approved
krusche
deleted the
bugfix/adaptive-learning/fix-attachment-unit-competency-links
branch
Checklist
General
Server
Motivation and Context
Fixes this issue: #9666
Description
Credits to @badkeyy in #9684
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
Performance Tests
Screenshots
Summary by CodeRabbit
New Features
Bug Fixes
Tests