Lectures: Merge attachment and video unit
#10575
Conversation
…:ls1intum/Artemis into feature/iris/video-transcription-storage
…:ls1intum/Artemis into feature/iris/video-transcription-storage
…:ls1intum/Artemis into feature/iris/video-transcription-storage
…:ls1intum/Artemis into feature/iris/video-transcription-storage
…ombine-video-unit-and-attachment-unit
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||
…ombine-video-unit-and-attachment-unit
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/main/java/de/tum/cit/aet/artemis/lecture/service/AttachmentVideoUnitService.java (3)
33-35: Add @transactional annotation for data consistency.Methods like
createAttachmentVideoUnit,updateAttachmentVideoUnit, andhandleStudentVersionFileperform operations across multiple repositories and the file system. Without transactional boundaries, runtime exceptions could leave the system in an inconsistent state.The coding guidelines mention "avoid_transactions" but this appears to refer to avoiding unnecessary transactions. For operations that modify multiple data sources, transactions are essential for consistency.
@Profile(PROFILE_CORE) @Service +@Transactional public class AttachmentVideoUnitService {
103-156: 🛠️ Refactor suggestionRefactor large method to improve maintainability.
The
updateAttachmentVideoUnitmethod is 54 lines long and handles multiple responsibilities: metadata updates, competency link management, attachment creation/updates, file handling, and external API calls. This violates the "small_methods" principle from the coding guidelines.Consider extracting the attachment handling logic into separate private methods:
public AttachmentVideoUnit updateAttachmentVideoUnit(AttachmentVideoUnit existingAttachmentVideoUnit, AttachmentVideoUnit updateUnit, Attachment updateAttachment, MultipartFile updateFile, boolean keepFilename, List<HiddenPageInfoDTO> hiddenPages, List<SlideOrderDTO> pageOrder) { Set<CompetencyLectureUnitLink> existingCompetencyLinks = new HashSet<>(existingAttachmentVideoUnit.getCompetencyLinks()); - existingAttachmentVideoUnit.setDescription(updateUnit.getDescription()); - existingAttachmentVideoUnit.setName(updateUnit.getName()); - existingAttachmentVideoUnit.setReleaseDate(updateUnit.getReleaseDate()); - existingAttachmentVideoUnit.setCompetencyLinks(updateUnit.getCompetencyLinks()); - existingAttachmentVideoUnit.setVideoSource(updateUnit.getVideoSource()); + updateUnitMetadata(existingAttachmentVideoUnit, updateUnit); - Attachment existingAttachment = existingAttachmentVideoUnit.getAttachment(); - - if (existingAttachment == null && updateAttachment != null) { - createAttachment(updateAttachment, existingAttachmentVideoUnit, updateFile, keepFilename); - } + handleAttachmentUpdate(existingAttachmentVideoUnit, updateAttachment, updateFile, keepFilename, hiddenPages, pageOrder); AttachmentVideoUnit savedAttachmentVideoUnit = lectureUnitService.saveWithCompetencyLinks(existingAttachmentVideoUnit, attachmentVideoUnitRepository::saveAndFlush); - // Set the original competencies back to the attachment video unit so that the competencyProgressService can determine which competencies changed - existingAttachmentVideoUnit.setCompetencyLinks(existingCompetencyLinks); - competencyProgressApi.ifPresent(api -> api.updateProgressForUpdatedLearningObjectAsync(existingAttachmentVideoUnit, Optional.of(updateUnit))); + updateCompetencyProgress(existingAttachmentVideoUnit, existingCompetencyLinks, updateUnit); + notifyExternalServices(savedAttachmentVideoUnit); return savedAttachmentVideoUnit; }
196-203: 🛠️ Refactor suggestionEnhance file validation for security.
The
handleFilemethod only checks if the file is not null and not empty, but doesn't validate file types, sizes, or other security constraints. This could pose security risks with malicious file uploads.Consider adding comprehensive file validation:
private void handleFile(MultipartFile file, Attachment attachment, boolean keepFilename, Long attachmentVideoUnitId) { if (file != null && !file.isEmpty()) { + validateFile(file); Path basePath = FilePathConverter.getAttachmentVideoUnitFileSystemPath().resolve(attachmentVideoUnitId.toString()); Path savePath = fileService.saveFile(file, basePath, FilePathType.ATTACHMENT_UNIT, keepFilename); attachment.setLink(FilePathConverter.externalUriForFileSystemPath(savePath, FilePathType.ATTACHMENT_UNIT, attachmentVideoUnitId).toString()); attachment.setUploadDate(ZonedDateTime.now()); } } + +private void validateFile(MultipartFile file) { + // Validate file type, size, and other security constraints + String contentType = file.getContentType(); + long size = file.getSize(); + // Add validation logic here +}
🧹 Nitpick comments (2)
src/main/java/de/tum/cit/aet/artemis/lecture/service/AttachmentVideoUnitService.java (2)
104-104: Simplify method signature.The method signature has 7 parameters, which makes it complex and hard to use. Consider creating a parameter object or DTO to encapsulate the update data.
public class AttachmentVideoUnitUpdateRequest { private AttachmentVideoUnit updateUnit; private Attachment updateAttachment; private MultipartFile updateFile; private boolean keepFilename; private List<HiddenPageInfoDTO> hiddenPages; private List<SlideOrderDTO> pageOrder; // constructors, getters, setters } public AttachmentVideoUnit updateAttachmentVideoUnit(AttachmentVideoUnit existingAttachmentVideoUnit, AttachmentVideoUnitUpdateRequest request) { // method implementation }
250-253: Review the client preparation method.The
prepareAttachmentVideoUnitForClientmethod sets collections to null, which might cause issues if the client code expects empty collections instead of null values. Consider setting empty collections or documenting this behavior clearly.public void prepareAttachmentVideoUnitForClient(AttachmentVideoUnit attachmentVideoUnit) { - attachmentVideoUnit.getLecture().setLectureUnits(null); - attachmentVideoUnit.getLecture().setAttachments(null); + attachmentVideoUnit.getLecture().setLectureUnits(Collections.emptyList()); + attachmentVideoUnit.getLecture().setAttachments(Collections.emptyList()); }Alternatively, add clear documentation about why null is preferred over empty collections.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/main/resources/config/liquibase/master.xmlis excluded by!**/*.xml
📒 Files selected for processing (9)
gradle/jacoco.gradle(1 hunks)src/main/java/de/tum/cit/aet/artemis/lecture/service/AttachmentVideoUnitService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/lecture/service/SlideSplitterService.java(11 hunks)src/main/java/de/tum/cit/aet/artemis/lecture/web/AttachmentVideoUnitResource.java(6 hunks)src/main/webapp/app/communication/posting-content/posting-content-part/posting-content-part.component.spec.ts(3 hunks)src/main/webapp/app/lecture/manage/pdf-preview/pdf-preview.component.spec.ts(23 hunks)src/main/webapp/app/lecture/manage/pdf-preview/pdf-preview.component.ts(18 hunks)src/test/java/de/tum/cit/aet/artemis/lecture/AttachmentVideoUnitIntegrationTest.java(14 hunks)src/test/java/de/tum/cit/aet/artemis/lecture/service/SlideSplitterServiceTest.java(35 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- gradle/jacoco.gradle
- src/main/webapp/app/communication/posting-content/posting-content-part/posting-content-part.component.spec.ts
- src/main/webapp/app/lecture/manage/pdf-preview/pdf-preview.component.spec.ts
- src/test/java/de/tum/cit/aet/artemis/lecture/AttachmentVideoUnitIntegrationTest.java
- src/main/java/de/tum/cit/aet/artemis/lecture/service/SlideSplitterService.java
- src/main/java/de/tum/cit/aet/artemis/lecture/web/AttachmentVideoUnitResource.java
- src/main/webapp/app/lecture/manage/pdf-preview/pdf-preview.component.ts
- src/test/java/de/tum/cit/aet/artemis/lecture/service/SlideSplitterServiceTest.java
🧰 Additional context used
📓 Path-based instructions (1)
`src/main/java/**/*.java`: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,de...
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/service/AttachmentVideoUnitService.java
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Build and Push Docker Image / Build Docker Image for ls1intum/artemis
- GitHub Check: Build and Push Docker Image / Build Docker Image for ls1intum/artemis
- GitHub Check: Build .war artifact
- GitHub Check: client-tests-selected
- GitHub Check: server-tests
- GitHub Check: client-tests
- GitHub Check: Analyse
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/main/java/de/tum/cit/aet/artemis/lecture/service/AttachmentVideoUnitService.java (3)
33-35: Add @transactional annotation for data consistencyThe service methods perform database operations and file system modifications that should be transactional to prevent inconsistent state on exceptions.
103-157: Break down this complex method into smaller focused methodsThis method is too large (55+ lines) and handles multiple responsibilities, violating the "small_methods" principle from coding guidelines. Consider extracting attachment handling, file processing, and slide splitting into separate private methods.
197-204: Add comprehensive file validation for securityThe method only checks for file emptiness but lacks file type validation, size limits, and other security measures to prevent malicious uploads.
🧹 Nitpick comments (1)
src/main/java/de/tum/cit/aet/artemis/lecture/service/AttachmentVideoUnitService.java (1)
103-104: Consider reducing parameter count with DTOsThe method has 7 parameters, which makes it difficult to use and maintain. Consider grouping related parameters into DTOs (e.g., UpdateAttachmentVideoUnitRequest).
+public class UpdateAttachmentVideoUnitRequest { + private final AttachmentVideoUnit updateUnit; + private final Attachment updateAttachment; + private final MultipartFile updateFile; + private final boolean keepFilename; + private final List<HiddenPageInfoDTO> hiddenPages; + private final List<SlideOrderDTO> pageOrder; + // constructor and getters +} -public AttachmentVideoUnit updateAttachmentVideoUnit(AttachmentVideoUnit existingAttachmentVideoUnit, AttachmentVideoUnit updateUnit, Attachment updateAttachment, - MultipartFile updateFile, boolean keepFilename, List<HiddenPageInfoDTO> hiddenPages, List<SlideOrderDTO> pageOrder) { +public AttachmentVideoUnit updateAttachmentVideoUnit(AttachmentVideoUnit existingAttachmentVideoUnit, UpdateAttachmentVideoUnitRequest request) {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
jest.config.js(1 hunks)src/main/java/de/tum/cit/aet/artemis/lecture/service/AttachmentVideoUnitService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/lecture/web/AttachmentVideoUnitResource.java(6 hunks)src/main/webapp/app/lecture/manage/pdf-preview/pdf-preview.component.spec.ts(23 hunks)src/test/java/de/tum/cit/aet/artemis/lecture/AttachmentVideoUnitIntegrationTest.java(14 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- jest.config.js
- src/main/webapp/app/lecture/manage/pdf-preview/pdf-preview.component.spec.ts
- src/test/java/de/tum/cit/aet/artemis/lecture/AttachmentVideoUnitIntegrationTest.java
- src/main/java/de/tum/cit/aet/artemis/lecture/web/AttachmentVideoUnitResource.java
🧰 Additional context used
📓 Path-based instructions (1)
`src/main/java/**/*.java`: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,de...
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/service/AttachmentVideoUnitService.java
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: client-tests
- GitHub Check: client-tests-selected
- GitHub Check: server-tests
- GitHub Check: Build and Push Docker Image / Build Docker Image for ls1intum/artemis
- GitHub Check: Build and Push Docker Image / Build Docker Image for ls1intum/artemis
- GitHub Check: Build .war artifact
- GitHub Check: Analyse
🔇 Additional comments (3)
src/main/java/de/tum/cit/aet/artemis/lecture/service/AttachmentVideoUnitService.java (3)
73-89: Well-structured creation methodThe method follows good practices by handling the persistence order correctly and integrating with external services appropriately.
159-169: Well-structured helper methodThe
createAttachmentmethod follows single responsibility principle and handles the attachment creation workflow cleanly.
214-231: Good file cleanup implementationThe
handleStudentVersionFilemethod properly handles cleanup of old files before saving new ones, demonstrating good resource management practices.
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||
bassner
deleted the
feature/lectures/combine-video-unit-and-attachment-unit
branch
github-project-automation
bot
moved this from Ready For Review
to Merged
in Artemis Development
Checklist
General
Server
Client
authoritiesto all new routes and checked the course groups for displaying navigation elements (links, buttons).Motivation and Context
To enable the linking of attachments to lecture transcriptions, the video unit and attachment unit have been merged into a single attachment-video unit. In the future, this unit can be sent to Pyris to simultaneously ingest transcriptions and slides. This integration is essential for providing Iris with the best context when answering questions about specific lecture topics.
Description
Steps for Testing
Prerequisites:
Testserver States
You can manage test servers using Helios. Check environment statuses in the environment list. To deploy to a test server, go to the CI/CD page, find your PR or branch, and trigger the deployment.
Review Progress
Performance Review
Code Review
Manual Tests
Test Coverage
Screenshots
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Documentation
Tests