Lectures: Prefer attachment units over attachments
#10708
Conversation
github-actions
bot
added
tests
server
github-actions
bot
added
communication
github-actions
bot
added
the
programming
WalkthroughThis set of changes removes the ability to create attachments directly via file upload in the lecture management system. The backend endpoint and service methods for creating attachments with files are deleted, and the corresponding UI elements and logic for adding new attachments are removed or replaced with navigation to a different creation route. The lecture and slide retrieval logic is refactored to use DTOs for controlled data exposure, with filtering for visible slides and attachment units. Related test cases for attachment creation and lecture slide retrieval are removed or updated. Additionally, some dependency injection patterns are modernized, and translation strings are updated for clarity. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI
participant Backend
participant DB
User->>UI: Navigate to lecture attachments
UI->>Backend: GET /lectures-with-slides
Backend->>DB: Query lectures, attachments, attachment units
Backend->>DB: Query visible slides for attachment units
Backend->>UI: Return List<LectureDTO> (with filtered, visible slides)
UI->>User: Display lectures, attachments, and slides
User->>UI: Click "Add attachment as lecture unit"
UI->>UI: Navigate to /unit-management/attachment-units/create
Possibly related PRs
Suggested labels
Suggested reviewers
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (9)
🪧 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: 1
🔭 Outside diff range comments (1)
src/main/java/de/tum/cit/aet/artemis/lecture/repository/LectureRepository.java (1)
46-54:⚠️ Potential issue
lu.attachmentbreaks the JPA type‑system – useTREATandDISTINCT
lecture.lectureUnitsis a polymorphic association of the abstract typeLectureUnit.
Dereferencinglu.attachmentis illegal JPA because that attribute exists only onAttachmentUnit.
Hibernate will accept it in lenient mode but other providers – or future Hibernate versions with strict compliance enabled – will throwIllegalArgumentException: could not resolve property: attachment.In addition, the extra joins introduce row duplication; relying on
Set<Lecture>to de‑duplicate happens in Java rather than SQL and wastes memory.-SELECT lecture +SELECT DISTINCT lecture ... - LEFT JOIN FETCH lecture.lectureUnits lu - LEFT JOIN FETCH lu.attachment + LEFT JOIN FETCH lecture.lectureUnits lu + LEFT JOIN FETCH TREAT(lu AS de.tum.cit.aet.artemis.lecture.domain.AttachmentUnit).attachmentThis keeps the query portable and reduces the result set size.
🧹 Nitpick comments (4)
jest.config.js (1)
94-95: Verify coverage threshold adjustments
The statements threshold has been lowered to 88.96% (from 88.98%) and branches to 75.15% (from 75.18%) to account for removed tests. Please confirm these new values accurately reflect current coverage reports and remain aligned with the TODO note on line 93 about eventually reaching 90%.src/main/java/de/tum/cit/aet/artemis/programming/service/localvc/ssh/HashUtils.java (1)
27-35: Well-implemented SHA-256 hashing method.The implementation is clean, with proper exception handling and documentation. The method follows good practices by using a standardized character encoding and leveraging the MessageDigest API correctly.
Minor inconsistency in error message wording:
- throw new IllegalStateException("Failed to hash token", e); + throw new IllegalStateException("Failed to hash secret", e);The parameter is named "secret" but the error message refers to "token".
src/main/java/de/tum/cit/aet/artemis/lecture/domain/LectureUnit.java (1)
114-116: Consider documenting / mitigating eager loading via@OneToOneinstead of omitting the association altogetherThe comment explains why
LectureTranscriptionis left out, but losing the navigation property forces every consumer to reach into the repository layer whenever they need the transcription.
With Hibernate 6 you can keep the property if you:
- Declare it with
@OneToOne(fetch = FetchType.LAZY, optional = true)and- Enable byte‑code enhancement (already used in Artemis for other lazy
@OneToOnes).This preserves the domain model without incurring unnecessary queries.
src/main/java/de/tum/cit/aet/artemis/lecture/web/LectureResource.java (1)
260-267:AttachmentUnitDTO.fromleaks mutable state via a sharedArrayList
slidesis created withnew ArrayList<>()and then exposed directly. Callers can mutate it inadvertently, breaking immutability expectations of a record.
ReturnList.of()and let the assembly code replace the list when necessary:-return new AttachmentUnitDTO(attachmentUnit.getId(), attachmentUnit.getName(), new ArrayList<>(), … +return new AttachmentUnitDTO(attachmentUnit.getId(), attachmentUnit.getName(), new ArrayList<>(), …) // <- if you keep itor
return new AttachmentUnitDTO( attachmentUnit.getId(), attachmentUnit.getName(), Collections.emptyList(), AttachmentDTO.from(attachmentUnit.getAttachment()), attachmentUnit.getReleaseDate(), "attachment");
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
docs/admin/accessRights.rst(0 hunks)docs/dev/guidelines/client-design.rst(1 hunks)jest.config.js(1 hunks)src/main/java/de/tum/cit/aet/artemis/lecture/domain/LectureTranscription.java(0 hunks)src/main/java/de/tum/cit/aet/artemis/lecture/domain/LectureUnit.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/lecture/dto/SlideDTO.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/lecture/repository/LectureRepository.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/lecture/repository/SlideRepository.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/lecture/service/LectureService.java(0 hunks)src/main/java/de/tum/cit/aet/artemis/lecture/web/AttachmentResource.java(0 hunks)src/main/java/de/tum/cit/aet/artemis/lecture/web/LectureResource.java(8 hunks)src/main/java/de/tum/cit/aet/artemis/lecture/web/LectureTranscriptionResource.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/localvc/ssh/HashUtils.java(1 hunks)src/main/webapp/app/communication/posting-reactions-bar/posting-reactions-bar.component.html(1 hunks)src/main/webapp/app/core/course/shared/course-sidebar/course-sidebar.component.html(1 hunks)src/main/webapp/app/core/user/settings/user-settings-container/user-settings-container.component.spec.ts(2 hunks)src/main/webapp/app/lecture/manage/lecture-attachments/lecture-attachments.component.html(1 hunks)src/main/webapp/app/lecture/manage/lecture-attachments/lecture-attachments.component.spec.ts(0 hunks)src/main/webapp/app/lecture/manage/lecture-attachments/lecture-attachments.component.ts(2 hunks)src/main/webapp/app/lecture/manage/lecture/lecture.component.spec.ts(2 hunks)src/main/webapp/app/lecture/manage/lecture/lecture.component.ts(2 hunks)src/main/webapp/app/lecture/manage/services/attachment.service.spec.ts(0 hunks)src/main/webapp/app/lecture/manage/services/attachment.service.ts(0 hunks)src/main/webapp/app/lecture/manage/services/lecture.service.spec.ts(0 hunks)src/main/webapp/app/lecture/manage/services/lecture.service.ts(0 hunks)src/main/webapp/app/lecture/overview/course-lectures/lecture-unit/lecture-unit.component.ts(2 hunks)src/main/webapp/app/tutorialgroup/shared/pipe/meeting-pattern.pipe.spec.ts(5 hunks)src/main/webapp/app/tutorialgroup/shared/pipe/meeting-pattern.pipe.ts(2 hunks)src/main/webapp/i18n/de/global.json(1 hunks)src/main/webapp/i18n/en/global.json(1 hunks)src/test/java/de/tum/cit/aet/artemis/lecture/AttachmentResourceIntegrationTest.java(0 hunks)src/test/java/de/tum/cit/aet/artemis/lecture/LectureIntegrationTest.java(0 hunks)
💤 Files with no reviewable changes (11)
- src/main/java/de/tum/cit/aet/artemis/lecture/domain/LectureTranscription.java
- src/main/webapp/app/lecture/manage/services/lecture.service.spec.ts
- src/main/webapp/app/lecture/manage/services/lecture.service.ts
- src/main/webapp/app/lecture/manage/services/attachment.service.ts
- src/main/java/de/tum/cit/aet/artemis/lecture/service/LectureService.java
- src/main/java/de/tum/cit/aet/artemis/lecture/web/AttachmentResource.java
- src/test/java/de/tum/cit/aet/artemis/lecture/AttachmentResourceIntegrationTest.java
- src/main/webapp/app/lecture/manage/lecture-attachments/lecture-attachments.component.spec.ts
- docs/admin/accessRights.rst
- src/test/java/de/tum/cit/aet/artemis/lecture/LectureIntegrationTest.java
- src/main/webapp/app/lecture/manage/services/attachment.service.spec.ts
🧰 Additional context used
📓 Path-based instructions (4)
`src/main/webapp/**/*.ts`: angular_style:https://angular.io/guide/styleguide;methods_in_html:false;lazy_loading:true;code_reuse:true;tests:meaningful;types:PascalCase;enums:PascalC...
src/main/webapp/app/core/user/settings/user-settings-container/user-settings-container.component.spec.tssrc/main/webapp/app/lecture/overview/course-lectures/lecture-unit/lecture-unit.component.tssrc/main/webapp/app/lecture/manage/lecture/lecture.component.tssrc/main/webapp/app/lecture/manage/lecture/lecture.component.spec.tssrc/main/webapp/app/tutorialgroup/shared/pipe/meeting-pattern.pipe.spec.tssrc/main/webapp/app/tutorialgroup/shared/pipe/meeting-pattern.pipe.tssrc/main/webapp/app/lecture/manage/lecture-attachments/lecture-attachments.component.ts
`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/**/*.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/communication/posting-reactions-bar/posting-reactions-bar.component.htmlsrc/main/webapp/app/core/course/shared/course-sidebar/course-sidebar.component.htmlsrc/main/webapp/app/lecture/manage/lecture-attachments/lecture-attachments.component.html
`src/main/webapp/i18n/de/**/*.json`: German language translations should be informal (dutzen) and should never be formal (sietzen). So the user should always be addressed with "du/...
src/main/webapp/i18n/de/**/*.json: German language translations should be informal (dutzen) and should never be formal (sietzen). So the user should always be addressed with "du/dein" and never with "sie/ihr".
src/main/webapp/i18n/de/global.json
`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/web/LectureTranscriptionResource.javasrc/main/java/de/tum/cit/aet/artemis/programming/service/localvc/ssh/HashUtils.javasrc/main/java/de/tum/cit/aet/artemis/lecture/dto/SlideDTO.javasrc/main/java/de/tum/cit/aet/artemis/lecture/repository/LectureRepository.javasrc/main/java/de/tum/cit/aet/artemis/lecture/domain/LectureUnit.javasrc/main/java/de/tum/cit/aet/artemis/lecture/repository/SlideRepository.javasrc/main/java/de/tum/cit/aet/artemis/lecture/web/LectureResource.java
🧬 Code Graph Analysis (2)
src/main/webapp/app/core/user/settings/user-settings-container/user-settings-container.component.spec.ts (1)
src/test/javascript/spec/helpers/mocks/service/mock-profile.service.ts (1)
MockProfileService(4-22)
src/main/java/de/tum/cit/aet/artemis/lecture/dto/SlideDTO.java (1)
src/main/webapp/app/lecture/shared/entities/lecture-unit/slide.model.ts (1)
Slide(4-10)
🔇 Additional comments (23)
src/main/webapp/app/core/user/settings/user-settings-container/user-settings-container.component.spec.ts (1)
3-3: Proper test setup with ProfileService provider.The addition of ProfileService and its mock implementation to the test setup ensures consistent service provisioning, aligning with the PR's enhanced dependency injection patterns.
Also applies to: 5-5, 33-33
src/main/webapp/app/core/course/shared/course-sidebar/course-sidebar.component.html (1)
17-17: Good adoption of tracking by unique ID for performance optimization.Using
track courseItem.idis a performance best practice for Angular's change detection as it allows more efficient DOM updates by tracking stable identifiers rather than object identity.src/main/webapp/app/communication/posting-reactions-bar/posting-reactions-bar.component.html (1)
52-52: Good adoption of key-based tracking for performance optimization.Using
track reactionMetaData.keyensures efficient DOM updates by tracking stable identifiers rather than object references, which is important for the keyvalue pipe output.src/main/webapp/app/lecture/overview/course-lectures/lecture-unit/lecture-unit.component.ts (1)
1-1: Good modernization to standalone injection pattern.The change to use Angular's standalone
inject()function instead of constructor-based dependency injection aligns with modern Angular practices and the project's coding standards.Also applies to: 21-21
docs/dev/guidelines/client-design.rst (1)
212-212: Documentation accurately updated to reflect modern injection pattern.The example code now correctly demonstrates the preferred Angular dependency injection pattern using the standalone
inject()function instead of constructor injection, maintaining consistency with the actual implementation.src/main/webapp/app/tutorialgroup/shared/pipe/meeting-pattern.pipe.spec.ts (3)
1-24: Good modernization of test setup using TestBed.The test setup has been improved to use Angular's
TestBedfor proper dependency injection rather than manual mocking. This approach better mirrors real application behavior and makes the tests more maintainable.
32-33: Good adoption of Jest spies for cleaner testing.Using
jest.spyOnprovides better verification capabilities for method calls compared to direct mock implementations, making the test more robust and easier to maintain.Also applies to: 46-47, 61-62, 76-77
51-52: Properly updated method call verification to use spies.The assertions now correctly verify the calls to the translated service using the spy, which is more maintainable and provides clearer failure messages when tests fail.
Also applies to: 66-67, 81-82
src/main/webapp/app/tutorialgroup/shared/pipe/meeting-pattern.pipe.ts (2)
1-1: Good modernization of dependency injection.Replacing constructor-based injection with Angular's standalone
inject()function follows modern Angular practices and simplifies the code. This change aligns with Angular's push toward functional programming patterns.Also applies to: 17-17
22-22: Well-implemented feature extension with backward compatibility.The addition of the optional
includeRepetitionFrequencyparameter with:
- Proper JSDoc documentation
- Default value preserving backward compatibility
- Clean conditional implementation
This change provides flexibility while maintaining existing functionality for callers.
Also applies to: 25-25, 31-31
src/main/java/de/tum/cit/aet/artemis/lecture/repository/SlideRepository.java (2)
70-77: Good refactoring of unhideSlide with direct JPQLThe refactoring from an entity-based approach to a direct JPQL update query with
@Modifyingand@Transactionalannotations is a good performance improvement that avoids unnecessary entity loading and reduces database overhead.
79-85: Well-designed query for optimized slide retrievalThis new query method efficiently retrieves only visible slides as DTOs for the specified attachment units, filtering based on the hidden status. The use of DTOs and specific field selection aligns perfectly with the PR objective to improve performance for courses with multiple lectures and slides.
src/main/webapp/i18n/en/global.json (1)
248-248: Updated text clarifies the preferred attachment creation methodThe text change from "Add attachment" to "Add attachment as lecture unit" effectively communicates to users that attachments should now be created as lecture units, aligning with the PR objective to deprecate simple lecture attachments.
src/main/webapp/i18n/de/global.json (1)
248-248: Correctly updated German translation using informal languageThe German translation has been properly updated to "Anhang als Vorlesungseinheit hinzufügen" and correctly uses the informal form ("dutzen") as required by the coding guidelines. This maintains consistency with the English version and communicates the same information about creating attachments as lecture units.
src/main/webapp/app/lecture/manage/lecture/lecture.component.spec.ts (2)
132-139: Updated method name in test mockThe mock provider now uses
findAllByCourseIdinstead offindAllByCourseIdWithSlides, correctly reflecting the service method changes as part of the DTO-based optimization.
172-173: Updated spy to match new method nameThe test now correctly spies on
findAllByCourseIdinstead offindAllByCourseIdWithSlides, maintaining test validity after the service refactoring to use DTOs for improved performance.src/main/java/de/tum/cit/aet/artemis/lecture/web/LectureTranscriptionResource.java (2)
98-99: Optimized transcription replacement logic.Excellent approach to finding and deleting existing transcriptions by lectureUnitId before creating a new one. This change properly handles the removal of the direct bidirectional entity references between LectureUnit and LectureTranscription, shifting to a repository-based approach.
109-109: Fixed Javadoc URL path format.The corrected URL path format in the Javadoc improves documentation accuracy.
src/main/java/de/tum/cit/aet/artemis/lecture/dto/SlideDTO.java (1)
1-16: Well-designed DTO for performance optimization.This SlideDTO record follows best practices for DTOs:
- Uses Java record for immutability and compact syntax
- Includes JsonInclude annotation to optimize serialization
- Provides a clear static factory method for entity conversion
- Contains only necessary data fields without exposing entities
- Properly extracts nested entity IDs (attachmentUnitId)
The introduction of this DTO aligns perfectly with the PR objective of optimizing performance for courses with multiple lectures, attachment units, and slides by reducing data transfer between server and client.
src/main/webapp/app/lecture/manage/lecture/lecture.component.ts (2)
20-21: Improved import paths for consistency.Good refactoring of imports from relative to absolute paths starting with 'app/shared/...'. This change makes imports more consistent and follows Angular style guidelines.
Also applies to: 24-26, 28-28
184-184: Updated service method to align with backend changes.The update from
findAllByCourseIdWithSlidestofindAllByCourseIdcorrectly aligns with the backend refactoring where slide data is now handled with DTOs for better performance. This change supports the PR objective of optimizing database calls and reducing data transfer.src/main/webapp/app/lecture/manage/lecture-attachments/lecture-attachments.component.html (1)
198-203: Redirect to attachment units creation instead of direct attachment creation.This UI change correctly implements the PR's core objective of deprecating simple lecture attachments in favor of attachment units. By replacing the "Add Attachment" button with a router link to the attachment units creation page, the UI now guides instructors toward the preferred approach that offers enhanced functionality.
src/main/webapp/app/lecture/manage/lecture-attachments/lecture-attachments.component.ts (1)
7-23: Imports look consistent with the new “update‑only” flowObsolete symbols related to creating / viewing attachments (e.g.
faEye,AttachmentType) have been removed and the remaining imports compile fine. No further action required.Also applies to: 44-44
github-project-automation
bot
moved this from Work In Progress
to Ready For Review
in Artemis Development
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||||||||
eceeeren
left a comment
eceeeren
left a comment
There was a problem hiding this comment.
Tested locally, all of the functionalities for Attachments and Attachment Units seem to work corrrectly. I only found an issue with referencing a slide when some pages are hidden.
src/main/java/de/tum/cit/aet/artemis/lecture/service/LectureService.java
Show resolved
Hide resolved
ahmetsenturk
left a comment
ahmetsenturk
left a comment
There was a problem hiding this comment.
Tested locally, all functionalities work as described - the requests are pretty fast, and the references in the communication section work out as well (clicking on units automatically redirects to the attachment - pptx automatically downloads pdf opens the file in another tab) 👍
Co-authored-by: Patrick Bassner <[email protected]>
…ce.java Co-authored-by: Patrick Bassner <[email protected]>
Thanks for the feedback. I agree that we should remove it here |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/main/java/de/tum/cit/aet/artemis/lecture/web/LectureResource.java (1)
225-226: 🛠️ Refactor suggestionHandle empty attachment unit sets to avoid dialect-specific SQL issues.
When calling
slideRepository.findVisibleSlidesByAttachmentUnits()with an empty set, some databases might produce syntax errors with queries likeIN ().-Set<SlideDTO> slides = slideRepository.findVisibleSlidesByAttachmentUnits(attachmentUnitIds); +Set<SlideDTO> slides = attachmentUnitIds.isEmpty() + ? Set.of() + : slideRepository.findVisibleSlidesByAttachmentUnits(attachmentUnitIds);
🧹 Nitpick comments (1)
src/main/java/de/tum/cit/aet/artemis/lecture/web/LectureResource.java (1)
229-240: Consider adding null checks for data structure access.When accessing the map with
slidesByAttachmentUnitId.get(attachmentUnitDTO.id), you're correctly checking for null, but consider usinggetOrDefaultfor more concise code.-List<SlideDTO> slidesForAttachmentUnit = slidesByAttachmentUnitId.get(attachmentUnitDTO.id); -if (slidesForAttachmentUnit != null) { - // remove unnecessary fields from the slide DTOs - var finalSlides = slidesForAttachmentUnit.stream() - .map(slideDTO -> new SlideDTO(slideDTO.id(), slideDTO.slideNumber(), null, null)) - .sorted(Comparator.comparingInt(SlideDTO::slideNumber)).toList(); - attachmentUnitDTO.slides.addAll(finalSlides); -} +List<SlideDTO> slidesForAttachmentUnit = slidesByAttachmentUnitId.getOrDefault(attachmentUnitDTO.id, List.of()); +if (!slidesForAttachmentUnit.isEmpty()) { + // remove unnecessary fields from the slide DTOs + var finalSlides = slidesForAttachmentUnit.stream() + .map(slideDTO -> new SlideDTO(slideDTO.id(), slideDTO.slideNumber(), null, null)) + .sorted(Comparator.comparingInt(SlideDTO::slideNumber)).toList(); + attachmentUnitDTO.slides.addAll(finalSlides); +}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/de/tum/cit/aet/artemis/lecture/domain/LectureUnit.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/lecture/repository/SlideRepository.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/lecture/web/LectureResource.java(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/de/tum/cit/aet/artemis/lecture/domain/LectureUnit.java
- src/main/java/de/tum/cit/aet/artemis/lecture/repository/SlideRepository.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/web/LectureResource.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 (6)
src/main/java/de/tum/cit/aet/artemis/lecture/web/LectureResource.java (6)
53-53: Good security enhancement with permission annotation.The shift to using
@EnforceAtLeastStudentInCourseis appropriate and aligns with modern authorization patterns, making security enforcement more declarative.
81-81: Good dependency management with constructor injection.Adding the SlideRepository dependency through constructor injection is a clean pattern that maintains proper dependency inversion. The use of constructor injection instead of field injection follows best practices.
Also applies to: 108-119
216-246: Performance and data transfer optimization looks good.The implementation efficiently retrieves only necessary data with several notable optimizations:
- Proper filtering of visible lectures for students
- Separate loading of slides to avoid large data exchanges
- Grouping slides by attachment unit for efficient organization
- Stripping unnecessary fields from DTOs
- Good performance logging to track execution time
249-267: Good DTO design with Java records.The use of Java records for DTOs is an excellent choice that:
- Reduces boilerplate code
- Provides immutability
- Has controlled serialization with
@JsonInclude(JsonInclude.Include.NON_EMPTY)- Clear transformation logic with static factory methods
- Proper filtering for student visibility
268-283: DTO hierarchy maintains single responsibility.Both
AttachmentDTOandAttachmentUnitDTOfollow single responsibility principle, each representing a distinct domain concept with appropriate transformations.
416-422: Modern Java switch expression is a great improvement.The refactoring to use the switch expression instead of multiple if-else statements improves readability and reduces the potential for bugs by enforcing exhaustive pattern matching.
|
Thanks everyone for testing and revieweing |
helios-aet
bot
temporarily deployed
to
artemis-test2.artemis.cit.tum.de
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||
github-project-automation
bot
moved this from Ready For Review
to Merged
in Artemis Development
We want to deprecate simple lecture attachments in the future and guide instructors to use attachment units, because they offer much more functionality, e.g.:
This PR is a first step and removes adding simple lecture attachments. Instead, the UI forwards instructors to the creation of attachment units. Old simple lecture attachments are still accessible for instructors and students. Instructors can edit and delete them, but cannot create new ones.
In addition, this PR significantly improves the performance in a few places, in particular for courses with multiple lectures, attachment units and slides. I created DTOs and improve database calls to minimize data transfer between server and database and between server and client for the
lectures-with-slidesREST call.Checklist
General
Server
Client
authoritiesto all new routes and checked the course groups for displaying navigation elements (links, buttons).Steps for Testing
Prerequisites:
lectures-with-slidesis super fastTestserver 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
Code Review
Manual Tests
Test Coverage
No real changes. I remove functionality and made other functionality faster.
Screenshots
New button:
Adding slides in a channel
Fast REST call despite its size and data:
Realized with a custom DTO
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
inject()function.Chores