Lectures: Fix the order of lecture units when merging pdf files (#6653)

src/main/java/de/tum/in/www1/artemis/domain/lecture/LectureUnit.java

@@ -37,12 +37,6 @@ public abstract class LectureUnit extends DomainObject implements LearningObject
     @Column(name = "release_date")
     protected ZonedDateTime releaseDate;
 
-    // This is explicitly required by Hibernate for the indexed collection (OrderColumn)
-    // https://docs.jboss.org/hibernate/stable/annotations/reference/en/html_single/#entity-hibspec-collection-extratype-indexbidir
-    @Column(name = "lecture_unit_order")
-    @Transient
-    private int order;
-
     @ManyToOne
     @JoinColumn(name = "lecture_id")
     @Cache(usage = CacheConcurrencyStrategy.NONSTRICT_READ_WRITE)
@@ -67,10 +61,6 @@ public void setName(String name) {
         this.name = name != null ? name.strip() : null;
     }
 
-    public int getOrder() {
-        return order;
-    }
-
     public Lecture getLecture() {
         return lecture;
     }

src/main/java/de/tum/in/www1/artemis/repository/AttachmentUnitRepository.java

@@ -1,6 +1,6 @@
 package de.tum.in.www1.artemis.repository;
 
-import java.util.Set;
+import java.util.List;
 
 import javax.validation.constraints.NotNull;
 
@@ -21,23 +21,29 @@ public interface AttachmentUnitRepository extends JpaRepository<AttachmentUnit,
 
     @Query("""
             SELECT attachmentUnit
-            FROM AttachmentUnit attachmentUnit
-            WHERE attachmentUnit.lecture.id = :#{#lectureId}
-            AND attachmentUnit.attachment.attachmentType = :#{#attachmentType}
+            FROM Lecture lecture
+                LEFT JOIN TREAT(lecture.lectureUnits as AttachmentUnit) attachmentUnit
+                LEFT JOIN FETCH attachmentUnit.attachment attachment
+            WHERE lecture.id = :lectureId
+                AND attachment.attachmentType = :attachmentType
+            ORDER BY INDEX(attachmentUnit)
             """)
-    Set<AttachmentUnit> findAllByLectureIdAndAttachmentType(@Param("lectureId") Long lectureId, @Param("attachmentType") AttachmentType attachmentType);
+    // INDEX() is used to retrieve the order saved by @OrderColumn, see https://en.wikibooks.org/wiki/Java_Persistence/JPQL#Special_Operators
+    List<AttachmentUnit> findAllByLectureIdAndAttachmentType(@Param("lectureId") Long lectureId, @Param("attachmentType") AttachmentType attachmentType);
 
     /**
      * Find all attachment units by lecture id and attachment type or throw if ist is empty.
+     * The list is sorted according to the order of units in the lecture.
      *
      * @param lectureId      the id of the lecture
      * @param attachmentType the attachment type
      * @return the list of all attachment units with the given lecture id and attachment type
      * @throws EntityNotFoundException if no results are found
      */
-    default Set<AttachmentUnit> findAllByLectureIdAndAttachmentTypeElseThrow(@Param("lectureId") Long lectureId, @Param("attachmentType") AttachmentType attachmentType)
+    @NotNull
+    default List<AttachmentUnit> findAllByLectureIdAndAttachmentTypeElseThrow(@Param("lectureId") Long lectureId, @Param("attachmentType") AttachmentType attachmentType)
             throws EntityNotFoundException {
-        Set<AttachmentUnit> attachmentUnits = findAllByLectureIdAndAttachmentType(lectureId, attachmentType);
+        List<AttachmentUnit> attachmentUnits = findAllByLectureIdAndAttachmentType(lectureId, attachmentType);
         if (attachmentUnits.isEmpty()) {
             throw new EntityNotFoundException("AttachmentUnit");
         }

src/main/java/de/tum/in/www1/artemis/repository/LectureRepository.java

@@ -26,69 +26,69 @@ public interface LectureRepository extends JpaRepository<Lecture, Long> {
     @Query("""
             SELECT lecture
             FROM Lecture lecture
-            LEFT JOIN FETCH lecture.attachments
-            WHERE lecture.course.id = :#{#courseId}
+                LEFT JOIN FETCH lecture.attachments
+            WHERE lecture.course.id = :courseId
             """)
     Set<Lecture> findAllByCourseIdWithAttachments(@Param("courseId") Long courseId);
 
     @Query("""
             SELECT lecture
             FROM Lecture lecture
-            LEFT JOIN FETCH lecture.attachments
-            LEFT JOIN FETCH lecture.lectureUnits
-            WHERE lecture.course.id = :#{#courseId}
+                LEFT JOIN FETCH lecture.attachments
+                LEFT JOIN FETCH lecture.lectureUnits
+            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
+                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
-            LEFT JOIN FETCH lecture.posts
-            LEFT JOIN FETCH lecture.lectureUnits lu
-            LEFT JOIN FETCH lu.completedUsers cu
-            LEFT JOIN FETCH lu.competencies
-            LEFT JOIN FETCH lu.exercise exercise
-            LEFT JOIN FETCH exercise.competencies
-            WHERE lecture.id = :#{#lectureId}
+                LEFT JOIN FETCH lecture.posts
+                LEFT JOIN FETCH lecture.lectureUnits lu
+                LEFT JOIN FETCH lu.completedUsers cu
+                LEFT JOIN FETCH lu.competencies
+                LEFT JOIN FETCH lu.exercise exercise
+                LEFT JOIN FETCH exercise.competencies
+            WHERE lecture.id = :lectureId
             """)
     Optional<Lecture> findByIdWithPostsAndLectureUnitsAndCompetenciesAndCompletions(@Param("lectureId") Long lectureId);
 
     @Query("""
             SELECT lecture
             FROM Lecture lecture
-            LEFT JOIN FETCH lecture.lectureUnits lu
-            LEFT JOIN FETCH lu.competencies
-            LEFT JOIN FETCH lu.exercise exercise
-            LEFT JOIN FETCH exercise.competencies
-            WHERE lecture.id = :#{#lectureId}
+                LEFT JOIN FETCH lecture.lectureUnits lu
+                LEFT JOIN FETCH lu.competencies
+                LEFT JOIN FETCH lu.exercise exercise
+                LEFT JOIN FETCH exercise.competencies
+            WHERE lecture.id = :lectureId
             """)
     Optional<Lecture> findByIdWithLectureUnitsAndCompetencies(@Param("lectureId") Long lectureId);
 
     @Query("""
             SELECT lecture
             FROM Lecture lecture
-            LEFT JOIN FETCH lecture.lectureUnits
-            WHERE lecture.id = :#{#lectureId}
+                LEFT JOIN FETCH lecture.lectureUnits
+            WHERE lecture.id = :lectureId
             """)
     Optional<Lecture> findByIdWithLectureUnits(@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.lectureUnits lectureUnit
+                LEFT JOIN FETCH lectureUnit.attachment luAttachment
+                LEFT JOIN FETCH lectureUnit.slides slides
             WHERE lecture.id = :lectureId
             """)
     Optional<Lecture> findByIdWithLectureUnitsAndWithSlides(@Param("lectureId") Long lectureId);
@@ -107,9 +107,10 @@ public interface LectureRepository extends JpaRepository<Lecture, Long> {
      * @return Page with search results
      */
     @Query("""
-            SELECT lecture FROM Lecture lecture
+            SELECT lecture
+            FROM Lecture lecture
             WHERE (lecture.course.instructorGroupName IN :groups OR lecture.course.editorGroupName IN :groups)
-            AND (lecture.title LIKE %:partialTitle% OR lecture.course.title LIKE %:partialCourseTitle%)
+                AND (lecture.title LIKE %:partialTitle% OR lecture.course.title LIKE %:partialCourseTitle%)
             """)
     Page<Lecture> findByTitleInLectureOrCourseAndUserHasAccessToCourse(@Param("partialTitle") String partialTitle, @Param("partialCourseTitle") String partialCourseTitle,
             @Param("groups") Set<String> groups, Pageable pageable);

src/main/java/de/tum/in/www1/artemis/web/rest/FileResource.java

@@ -7,7 +7,6 @@
 import java.net.URLConnection;
 import java.nio.file.Path;
 import java.time.Duration;
-import java.util.Comparator;
 import java.util.List;
 import java.util.Optional;
 import java.util.Set;
@@ -32,7 +31,6 @@
 import de.tum.in.www1.artemis.domain.enumeration.ProjectType;
 import de.tum.in.www1.artemis.domain.exam.ExamUser;
 import de.tum.in.www1.artemis.domain.lecture.AttachmentUnit;
-import de.tum.in.www1.artemis.domain.lecture.LectureUnit;
 import de.tum.in.www1.artemis.domain.lecture.Slide;
 import de.tum.in.www1.artemis.domain.participation.StudentParticipation;
 import de.tum.in.www1.artemis.repository.*;
@@ -359,11 +357,10 @@ public ResponseEntity<byte[]> getLecturePdfAttachmentsMerged(@PathVariable Long
 
         authCheckService.checkHasAtLeastRoleForLectureElseThrow(Role.STUDENT, lecture, user);
 
-        Set<AttachmentUnit> lectureAttachments = attachmentUnitRepository.findAllByLectureIdAndAttachmentTypeElseThrow(lectureId, AttachmentType.FILE);
+        List<AttachmentUnit> lectureAttachments = attachmentUnitRepository.findAllByLectureIdAndAttachmentTypeElseThrow(lectureId, AttachmentType.FILE);
 
         List<String> attachmentLinks = lectureAttachments.stream()
                 .filter(unit -> authCheckService.isAllowedToSeeLectureUnit(unit, user) && "pdf".equals(StringUtils.substringAfterLast(unit.getAttachment().getLink(), ".")))
-                .sorted(Comparator.comparing(LectureUnit::getOrder))
                 .map(unit -> Path.of(FilePathService.getAttachmentUnitFilePath(), String.valueOf(unit.getId()), StringUtils.substringAfterLast(unit.getAttachment().getLink(), "/"))
                         .toString())
                 .toList();

src/test/java/de/tum/in/www1/artemis/FileIntegrationTest.java

@@ -6,10 +6,11 @@
 import java.nio.file.Path;
 import java.time.ZonedDateTime;
 import java.util.Comparator;
-import java.util.Set;
+import java.util.List;
 
 import org.apache.pdfbox.pdmodel.PDDocument;
 import org.apache.pdfbox.pdmodel.PDPage;
+import org.apache.pdfbox.pdmodel.common.PDRectangle;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.springframework.beans.factory.annotation.Autowired;
@@ -244,7 +245,7 @@ void testGetAttachmentUnit() throws Exception {
 
         MockMultipartFile file = new MockMultipartFile("file", "filename2.png", "application/json", "some data".getBytes());
         AttachmentUnit attachmentUnit = uploadAttachmentUnit(file, HttpStatus.CREATED);
-        database.addLectureUnitsToLecture(lecture, Set.of(attachmentUnit));
+        database.addLectureUnitsToLecture(lecture, List.of(attachmentUnit));
 
         String attachmentPath = attachmentUnit.getAttachment().getLink();
         String receivedAttachment = request.get(attachmentPath, HttpStatus.OK, String.class);
@@ -333,7 +334,7 @@ void uploadFileAsTutor() throws Exception {
     }
 
     @Test
-    @WithMockUser(username = TEST_PREFIX + "student1", roles = "TA")
+    @WithMockUser(username = TEST_PREFIX + "instructor1", roles = "INSTRUCTOR")
     void uploadFileUnsupportedFileExtension() throws Exception {
         // create file
         MockMultipartFile file = new MockMultipartFile("file", "something.exotic", "application/json", "some data".getBytes());
@@ -359,23 +360,68 @@ void testGetLecturePdfAttachmentsMerged() throws Exception {
     void testGetLecturePdfAttachmentsMerged_TutorAccessToUnreleasedUnits() throws Exception {
         Lecture lecture = createLectureWithLectureUnits();
 
+        adjustReleaseDateToFuture(lecture);
+
+        // The unit is hidden but a tutor can still see it
+        // -> the merged result should contain the unit
+        callAndCheckMergeResult(lecture, 5);
+    }
+
+    @Test
+    @WithMockUser(username = TEST_PREFIX + "tutor1", roles = "TA")
+    void testGetLecturePdfAttachmentsMerged_NoAccessToUnreleasedUnits() throws Exception {
+        Lecture lecture = createLectureWithLectureUnits();
+
+        adjustReleaseDateToFuture(lecture);
+
+        // The test setup needs at least TA right for creating a lecture with files.
+        database.changeUser(TEST_PREFIX + "student1");
+
+        // The unit is hidden, students should not see it in the merged result
+        callAndCheckMergeResult(lecture, 2);
+    }
+
+    private void adjustReleaseDateToFuture(Lecture lecture) {
         var attachment = lecture.getLectureUnits().stream().sorted(Comparator.comparing(LectureUnit::getId)).map(lectureUnit -> ((AttachmentUnit) lectureUnit).getAttachment())
                 .findFirst().orElseThrow();
         attachment.setReleaseDate(ZonedDateTime.now().plusHours(2));
         attachmentRepo.save(attachment);
+    }
 
-        // The unit is hidden but a tutor can still see it
-        // -> the merged result should contain the unit
-        callAndCheckMergeResult(lecture, 5);
+    @Test
+    @WithMockUser(username = TEST_PREFIX + "tutor1", roles = "TA")
+    void testGetLecturePdfAttachmentsMerged_correctOrder() throws Exception {
+        Lecture lecture = createLectureWithLectureUnits();
+
+        // Change order of units
+        LectureUnit unit3 = lecture.getLectureUnits().get(2);
+        lecture.getLectureUnits().remove(unit3);
+        lecture.getLectureUnits().add(0, unit3);
+        lectureRepo.save(lecture);
+
+        // The test setup needs at least TA right for creating a lecture with files.
+        database.changeUser(TEST_PREFIX + "student1");
+
+        try (PDDocument mergedDoc = retrieveMergeResult(lecture)) {
+            assertThat(mergedDoc.getNumberOfPages()).isEqualTo(5);
+            PDPage firstPage = mergedDoc.getPage(0);
+            // Verify that attachment 3 (created with a special crop box in createLectureWithLectureUnits) was moved to the start
+            // and is now the first page of the merged pdf
+            assertThat(firstPage.getCropBox().getHeight()).isEqualTo(4);
+        }
     }
 
     private void callAndCheckMergeResult(Lecture lecture, int expectedPages) throws Exception {
+        try (PDDocument mergedDoc = retrieveMergeResult(lecture)) {
+            assertThat(mergedDoc.getNumberOfPages()).isEqualTo(expectedPages);
+        }
+    }
+
+    private PDDocument retrieveMergeResult(Lecture lecture) throws Exception {
         byte[] receivedFile = request.get("/api/files/attachments/lecture/" + lecture.getId() + "/merge-pdf", HttpStatus.OK, byte[].class);
 
         assertThat(receivedFile).isNotEmpty();
-        try (PDDocument mergedDoc = PDDocument.load(receivedFile)) {
-            assertThat(mergedDoc.getNumberOfPages()).isEqualTo(expectedPages);
-        }
+        return PDDocument.load(receivedFile);
     }
 
     private Lecture createLectureWithLectureUnits() throws Exception {
@@ -408,14 +454,17 @@ private Lecture createLectureWithLectureUnits(HttpStatus expectedStatus) throws
         // create pdf file 3
         AttachmentUnit unit3;
         try (ByteArrayOutputStream outputStream = new ByteArrayOutputStream(); PDDocument doc2 = new PDDocument()) {
-            doc2.addPage(new PDPage());
+            // Add first page with extra cropBox to make it distinguishable in the later tests
+            PDPage page = new PDPage();
+            page.setCropBox(new PDRectangle(1, 2, 3, 4));
+            doc2.addPage(page);
             doc2.addPage(new PDPage());
             doc2.save(outputStream);
             MockMultipartFile file3 = new MockMultipartFile("file", "filename3.pdf", "application/json", outputStream.toByteArray());
             unit3 = uploadAttachmentUnit(file3, expectedStatus);
         }
 
-        lecture = database.addLectureUnitsToLecture(lecture, Set.of(unit1, unit2, unit3));
+        lecture = database.addLectureUnitsToLecture(lecture, List.of(unit1, unit2, unit3));
 
         return lecture;
     }

src/test/java/de/tum/in/www1/artemis/assessment/ParticipantScoreIntegrationTest.java

@@ -108,7 +108,7 @@ void setupTestScenario() {
         courseId = course.getId();
         TextExercise textExercise = database.createIndividualTextExercise(course, pastTimestamp, pastTimestamp, pastTimestamp);
         ExerciseUnit exerciseUnit = database.createExerciseUnit(textExercise);
-        database.addLectureUnitsToLecture(lecture, Set.of(exerciseUnit));
+        database.addLectureUnitsToLecture(lecture, List.of(exerciseUnit));
         lecture = lectureRepository.findByIdWithLectureUnitsAndCompetenciesElseThrow(lecture.getId());
         exerciseUnit = (ExerciseUnit) lecture.getLectureUnits().get(0);
         idOfExerciseUnit = exerciseUnit.getId();

src/test/java/de/tum/in/www1/artemis/lecture/LectureIntegrationTest.java

@@ -65,7 +65,7 @@ void initTestCase() throws Exception {
         TextUnit textUnit = database.createTextUnit();
         addAttachmentToLecture();
 
-        this.lecture1 = database.addLectureUnitsToLecture(this.lecture1, Set.of(exerciseUnit, attachmentUnit, videoUnit, textUnit));
+        this.lecture1 = database.addLectureUnitsToLecture(this.lecture1, List.of(exerciseUnit, attachmentUnit, videoUnit, textUnit));
     }
 
     private void addAttachmentToLecture() {
@@ -158,7 +158,7 @@ void getLectureForCourse_WithLectureUnitsWithSlides_shouldGetLecturesWithLecture
         Lecture lectureWithSlides = ModelFactory.generateLecture(ZonedDateTime.now().minusDays(5), ZonedDateTime.now().plusDays(5), course1);
         lectureWithSlides = lectureRepository.save(lectureWithSlides);
         AttachmentUnit attachmentUnitWithSlides = database.createAttachmentUnitWithSlides(numberOfSlides);
-        lectureWithSlides = database.addLectureUnitsToLecture(lectureWithSlides, Set.of(attachmentUnitWithSlides));
+        lectureWithSlides = database.addLectureUnitsToLecture(lectureWithSlides, List.of(attachmentUnitWithSlides));
 
         List<Lecture> returnedLectures = request.getList("/api/courses/" + course1.getId() + "/lectures-with-slides", HttpStatus.OK, Lecture.class);
 

src/test/java/de/tum/in/www1/artemis/lecture/LectureUnitIntegrationTest.java

@@ -65,8 +65,8 @@ void initTestCase() throws Exception {
         // textUnit3 is not one of the lecture units connected to the lecture
         this.textUnit3 = database.createTextUnit();
 
-        database.addLectureUnitsToLecture(course1.getLectures().stream().skip(1).findFirst().get(), Set.of(textUnit2));
-        this.lecture1 = database.addLectureUnitsToLecture(this.lecture1, Set.of(this.textUnit, onlineUnit, attachmentUnit));
+        database.addLectureUnitsToLecture(course1.getLectures().stream().skip(1).findFirst().get(), List.of(textUnit2));
+        this.lecture1 = database.addLectureUnitsToLecture(this.lecture1, List.of(this.textUnit, onlineUnit, attachmentUnit));
         this.lecture1 = lectureRepository.findByIdWithLectureUnitsElseThrow(lecture1.getId());
         this.textUnit = textUnitRepository.findById(this.textUnit.getId()).get();
         this.textUnit2 = textUnitRepository.findById(textUnit2.getId()).get();

src/test/java/de/tum/in/www1/artemis/util/DatabaseUtilService.java

@@ -883,7 +883,7 @@ public List<Course> createCoursesWithExercisesAndLecturesAndLectureUnits(String
                 TextUnit textUnit = createTextUnit();
                 AttachmentUnit attachmentUnit = createAttachmentUnit(withFiles);
                 ExerciseUnit exerciseUnit = createExerciseUnit(textExercise);
-                lectures.set(i, addLectureUnitsToLecture(lectures.get(i), Set.of(videoUnit, textUnit, attachmentUnit, exerciseUnit)));
+                lectures.set(i, addLectureUnitsToLecture(lectures.get(i), List.of(videoUnit, textUnit, attachmentUnit, exerciseUnit)));
             }
             course.setLectures(new HashSet<>(lectures));
         }).toList();
@@ -908,7 +908,7 @@ public Lecture addCompetencyToLectureUnits(Lecture lecture, Set<Competency> comp
         return l;
     }
 
-    public Lecture addLectureUnitsToLecture(Lecture lecture, Set<LectureUnit> lectureUnits) {
+    public Lecture addLectureUnitsToLecture(Lecture lecture, List<LectureUnit> lectureUnits) {
         Lecture l = lectureRepo.findByIdWithLectureUnits(lecture.getId()).get();
         for (LectureUnit lectureUnit : lectureUnits) {
             l.addLectureUnit(lectureUnit);