Lectures: Prefer attachment units over attachments (#10708)

Author: krusche

docs/admin/accessRights.rst

@@ -197,27 +197,27 @@ Quiz Exercises
 
 Lectures
 --------
-+---------------------+------------+--------+--------------------+
-|                     | Instructor | Editor | Teaching Assistant |
-+---------------------+------------+--------+--------------------+
-| Add attachment      |      ✔     |    ✔   |                    |
-+---------------------+------------+--------+--------------------+
-| Edit attachment     |      ✔     |    ✔   |                    |
-+---------------------+------------+--------+--------------------+
-| Delete attachment   |      ✔     |        |                    |
-+---------------------+------------+--------+--------------------+
-| Add Lecture Unit    |      ✔     |    ✔   |                    |
-+---------------------+------------+--------+--------------------+
-| Edit Lecture Unit   |      ✔     |    ✔   |                    |
-+---------------------+------------+--------+--------------------+
-| Delete Lecture Unit |      ✔     |        |                    |
-+---------------------+------------+--------+--------------------+
-| Create Lecture      |      ✔     |    ✔   |                    |
-+---------------------+------------+--------+--------------------+
-| Edit Lecture        |      ✔     |    ✔   |                    |
-+---------------------+------------+--------+--------------------+
-| Delete Lecture      |      ✔     |        |                    |
-+---------------------+------------+--------+--------------------+
++--------------------------+------------+--------+--------------------+
+|                          | Instructor | Editor | Teaching Assistant |
++--------------------------+------------+--------+--------------------+
+| Add attachment (in unit) |      ✔     |    ✔   |                    |
++--------------------------+------------+--------+--------------------+
+| Edit attachment          |      ✔     |    ✔   |                    |
++--------------------------+------------+--------+--------------------+
+| Delete attachment        |      ✔     |        |                    |
++--------------------------+------------+--------+--------------------+
+| Add Lecture Unit         |      ✔     |    ✔   |                    |
++--------------------------+------------+--------+--------------------+
+| Edit Lecture Unit        |      ✔     |    ✔   |                    |
++--------------------------+------------+--------+--------------------+
+| Delete Lecture Unit      |      ✔     |        |                    |
++--------------------------+------------+--------+--------------------+
+| Create Lecture           |      ✔     |    ✔   |                    |
++--------------------------+------------+--------+--------------------+
+| Edit Lecture             |      ✔     |    ✔   |                    |
++--------------------------+------------+--------+--------------------+
+| Delete Lecture           |      ✔     |        |                    |
++--------------------------+------------+--------+--------------------+
 
 Exam
 ----

docs/dev/guidelines/client-design.rst

@@ -209,7 +209,7 @@ For example, you could add a reactive flag to your component that indicates whet
         isDark = false;
         themeSubscription: Subscription;
 
-        constructor(private themeService: ThemeService) {}
+        private themeService = inject(ThemeService);
 
         ngOnInit() {
             this.themeSubscription = this.themeService.getCurrentThemeObservable().subscribe((theme) => {

jest.config.js

@@ -92,8 +92,8 @@ module.exports = {
         global: {
             // TODO: in the future, the following values should increase to at least 90%
             statements: 88.98,
-            branches: 75.18,
-            functions: 82.98,
+            branches: 75.15,
+            functions: 82.96,
             lines: 89.04,
         },
     },

src/main/java/de/tum/cit/aet/artemis/lecture/domain/LectureTranscription.java

@@ -10,8 +10,6 @@
 import jakarta.persistence.Table;
 import jakarta.validation.constraints.Size;
 
-import com.fasterxml.jackson.annotation.JsonIgnore;
-
 import de.tum.cit.aet.artemis.core.domain.DomainObject;
 
 @Entity
@@ -31,7 +29,6 @@ public class LectureTranscription extends DomainObject {
 
     @OneToOne
     @JoinColumn(name = "lecture_unit_id", unique = true)
-    @JsonIgnore
     private LectureUnit lectureUnit;
 
     public LectureTranscription() {

src/main/java/de/tum/cit/aet/artemis/lecture/domain/LectureUnit.java

@@ -17,7 +17,6 @@
 import jakarta.persistence.JoinColumn;
 import jakarta.persistence.ManyToOne;
 import jakarta.persistence.OneToMany;
-import jakarta.persistence.OneToOne;
 import jakarta.persistence.Table;
 import jakarta.persistence.Transient;
 
@@ -79,10 +78,6 @@ public abstract class LectureUnit extends DomainObject implements LearningObject
     @JsonIgnore // important, so that the completion status of other users do not leak to anyone
     private Set<LectureUnitCompletion> completedUsers = new HashSet<>();
 
-    @OneToOne(mappedBy = "lectureUnit")
-    @JsonIgnore
-    protected LectureTranscription lectureTranscription;
-
     public String getName() {
         return name;
     }
@@ -116,13 +111,7 @@ public void setCompetencyLinks(Set<CompetencyLectureUnitLink> competencyLinks) {
         this.competencyLinks = competencyLinks;
     }
 
-    public LectureTranscription getLectureTranscription() {
-        return lectureTranscription;
-    }
-
-    public void setLectureTranscription(LectureTranscription lectureTranscription) {
-        this.lectureTranscription = lectureTranscription;
-    }
+    // NOTE: we explicitly do not add LectureTranscription here to avoid Hibernate issues because of its OneToOne relationship which is EAGER and cannot be set to LAZY
 
     @JsonIgnore(false)
     @JsonProperty("completed")

src/main/java/de/tum/cit/aet/artemis/lecture/dto/SlideDTO.java

@@ -0,0 +1,15 @@
+package de.tum.cit.aet.artemis.lecture.dto;
+
+import java.time.ZonedDateTime;
+
+import com.fasterxml.jackson.annotation.JsonInclude;
+
+import de.tum.cit.aet.artemis.lecture.domain.Slide;
+
+@JsonInclude(JsonInclude.Include.NON_EMPTY)
+public record SlideDTO(Long id, int slideNumber, ZonedDateTime hidden, Long attachmentUnitId) {
+
+    public static SlideDTO from(Slide slide) {
+        return new SlideDTO(slide.getId(), slide.getSlideNumber(), slide.getHidden(), slide.getAttachmentUnit().getId());
+    }
+}

src/main/java/de/tum/cit/aet/artemis/lecture/repository/LectureRepository.java

@@ -47,22 +47,12 @@ public interface LectureRepository extends ArtemisJpaRepository<Lecture, Long> {
             SELECT lecture
             FROM Lecture lecture
                 LEFT JOIN FETCH lecture.attachments
-                LEFT JOIN FETCH lecture.lectureUnits
+                LEFT JOIN FETCH lecture.lectureUnits lu
+                LEFT JOIN FETCH lu.attachment
             WHERE lecture.course.id = :courseId
             """)
     Set<Lecture> findAllByCourseIdWithAttachmentsAndLectureUnits(@Param("courseId") Long courseId);
 
-    @Query("""
-            SELECT lecture
-            FROM Lecture lecture
-                LEFT JOIN FETCH lecture.attachments attachment
-                LEFT JOIN FETCH lecture.lectureUnits lectureUnit
-                LEFT JOIN FETCH lectureUnit.attachment luAttachment
-                LEFT JOIN FETCH lectureUnit.slides slides
-            WHERE lecture.course.id = :courseId
-            """)
-    Set<Lecture> findAllByCourseIdWithAttachmentsAndLectureUnitsAndSlides(@Param("courseId") Long courseId);
-
     @Query("""
             SELECT lecture
             FROM Lecture lecture
@@ -109,17 +99,6 @@ public interface LectureRepository extends ArtemisJpaRepository<Lecture, Long> {
             """)
     Optional<Lecture> findByIdWithLectureUnitsAndAttachments(@Param("lectureId") Long lectureId);
 
-    @Query("""
-            SELECT lecture
-            FROM Lecture lecture
-                LEFT JOIN FETCH lecture.lectureUnits lectureUnit
-                LEFT JOIN FETCH lectureUnit.attachment luAttachment
-                LEFT JOIN FETCH lectureUnit.slides slides
-                LEFT JOIN FETCH lecture.attachments
-            WHERE lecture.id = :lectureId
-            """)
-    Optional<Lecture> findByIdWithLectureUnitsAndSlidesAndAttachments(@Param("lectureId") long lectureId);
-
     @Query("""
             SELECT lecture
             FROM Lecture lecture
@@ -200,11 +179,6 @@ default Lecture findByIdWithLectureUnitsAndAttachmentsElseThrow(Long lectureId)
         return getValueElseThrow(findByIdWithLectureUnitsAndAttachments(lectureId), lectureId);
     }
 
-    @NotNull
-    default Lecture findByIdWithLectureUnitsAndSlidesAndAttachmentsElseThrow(long lectureId) {
-        return getValueElseThrow(findByIdWithLectureUnitsAndSlidesAndAttachments(lectureId), lectureId);
-    }
-
     @Query("""
             SELECT new de.tum.cit.aet.artemis.core.dto.CourseContentCount(
                 COUNT(l.id),

src/main/java/de/tum/cit/aet/artemis/lecture/repository/SlideRepository.java

@@ -3,15 +3,18 @@
 import static de.tum.cit.aet.artemis.core.config.Constants.PROFILE_CORE;
 
 import java.util.List;
+import java.util.Set;
 
 import org.springframework.context.annotation.Profile;
+import org.springframework.data.jpa.repository.Modifying;
 import org.springframework.data.jpa.repository.Query;
 import org.springframework.data.repository.query.Param;
 import org.springframework.stereotype.Repository;
 import org.springframework.transaction.annotation.Transactional;
 
 import de.tum.cit.aet.artemis.core.repository.base.ArtemisJpaRepository;
 import de.tum.cit.aet.artemis.lecture.domain.Slide;
+import de.tum.cit.aet.artemis.lecture.dto.SlideDTO;
 import de.tum.cit.aet.artemis.lecture.dto.SlideUnhideDTO;
 
 /**
@@ -64,11 +67,20 @@ public interface SlideRepository extends ArtemisJpaRepository<Slide, Long> {
      *
      * @param slideId The ID of the slide to unhide
      */
-    @Transactional
-    default void unhideSlide(Long slideId) {
-        findById(slideId).ifPresent(slide -> {
-            slide.setHidden(null);
-            save(slide);
-        });
-    }
+    @Transactional // ok because of modifying query
+    @Modifying
+    @Query("""
+            UPDATE Slide s
+            SET s.hidden = NULL
+            WHERE s.id = :slideId
+            """)
+    void unhideSlide(@Param("slideId") Long slideId);
+
+    @Query("""
+            SELECT new de.tum.cit.aet.artemis.lecture.dto.SlideDTO(s.id, s.slideNumber, s.hidden, s.attachmentUnit.id)
+            FROM Slide s
+            WHERE s.attachmentUnit.id IN :attachmentUnitIds
+                AND (s.hidden IS NULL OR s.hidden > CURRENT_TIMESTAMP())
+            """)
+    Set<SlideDTO> findVisibleSlidesByAttachmentUnits(@Param("attachmentUnitIds") Set<Long> attachmentUnitIds);
 }

src/main/java/de/tum/cit/aet/artemis/lecture/service/LectureService.java

@@ -31,7 +31,6 @@
 import de.tum.cit.aet.artemis.lecture.domain.Lecture;
 import de.tum.cit.aet.artemis.lecture.domain.LectureTranscription;
 import de.tum.cit.aet.artemis.lecture.domain.LectureUnit;
-import de.tum.cit.aet.artemis.lecture.domain.Slide;
 import de.tum.cit.aet.artemis.lecture.domain.VideoUnit;
 import de.tum.cit.aet.artemis.lecture.repository.LectureRepository;
 
@@ -211,18 +210,4 @@ public void ingestTranscriptionInPyris(LectureTranscription transcription, Cours
     public void deleteLectureTranscriptionInPyris(LectureTranscription existingLectureTranscription) {
         irisLectureApi.ifPresent(webhookService -> webhookService.deleteLectureTranscription(existingLectureTranscription));
     }
-
-    /**
-     * Filters the slides of all attachment units in a given lecture to exclude slides where `hidden` is not null.
-     *
-     * @param lectureWithAttachmentUnits the lecture containing attachment units
-     */
-    public void filterHiddenPagesOfAttachmentUnits(Lecture lectureWithAttachmentUnits) {
-        for (LectureUnit unit : lectureWithAttachmentUnits.getLectureUnits()) {
-            if (unit instanceof AttachmentUnit attachmentUnit) {
-                List<Slide> filteredSlides = attachmentUnit.getSlides().stream().filter(slide -> slide.getHidden() == null).toList();
-                attachmentUnit.setSlides(filteredSlides);
-            }
-        }
-    }
 }

src/main/java/de/tum/cit/aet/artemis/lecture/web/AttachmentResource.java

@@ -4,7 +4,6 @@
 import static de.tum.cit.aet.artemis.core.service.FilePathService.actualPathForPublicPath;
 
 import java.net.URI;
-import java.net.URISyntaxException;
 import java.nio.file.Path;
 import java.util.List;
 import java.util.Optional;
@@ -18,7 +17,6 @@
 import org.springframework.web.bind.annotation.DeleteMapping;
 import org.springframework.web.bind.annotation.GetMapping;
 import org.springframework.web.bind.annotation.PathVariable;
-import org.springframework.web.bind.annotation.PostMapping;
 import org.springframework.web.bind.annotation.PutMapping;
 import org.springframework.web.bind.annotation.RequestMapping;
 import org.springframework.web.bind.annotation.RequestParam;
@@ -77,29 +75,6 @@ public AttachmentResource(AttachmentRepository attachmentRepository, GroupNotifi
         this.fileService = fileService;
     }
 
-    /**
-     * POST /attachments : Create a new attachment.
-     *
-     * @param attachment the attachment object to create
-     * @param file       the file to save
-     * @return the ResponseEntity with status 201 (Created) and with body the new attachment, or with status 400 (Bad Request) if the attachment has already an ID
-     * @throws URISyntaxException if the Location URI syntax is incorrect
-     */
-    @PostMapping(value = "attachments", consumes = MediaType.MULTIPART_FORM_DATA_VALUE)
-    @EnforceAtLeastEditor
-    public ResponseEntity<Attachment> createAttachment(@RequestPart Attachment attachment, @RequestPart MultipartFile file) throws URISyntaxException {
-        log.debug("REST request to save Attachment : {}", attachment);
-        attachment.setId(null);
-
-        Path basePath = FilePathService.getLectureAttachmentFilePath().resolve(attachment.getLecture().getId().toString());
-        Path savePath = fileService.saveFile(file, basePath, true);
-        attachment.setLink(FilePathService.publicPathForActualPathOrThrow(savePath, attachment.getLecture().getId()).toString());
-
-        Attachment result = attachmentRepository.save(attachment);
-
-        return ResponseEntity.created(new URI("/api/lecture/attachments/" + result.getId())).body(result);
-    }
-
     /**
      * PUT /attachments/:id : Updates an existing attachment.
      *