Quiz exercises: Simplify drag-and-drop image upload

src/main/java/de/tum/in/www1/artemis/domain/quiz/DragAndDropQuestion.java

@@ -1,12 +1,15 @@
 package de.tum.in.www1.artemis.domain.quiz;
 
-import java.nio.file.Path;
+import java.net.URI;
 import java.util.*;
 
 import javax.persistence.*;
 
+import org.apache.commons.lang3.StringUtils;
 import org.hibernate.annotations.Cache;
 import org.hibernate.annotations.CacheConcurrencyStrategy;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 import com.fasterxml.jackson.annotation.JsonIgnore;
 import com.fasterxml.jackson.annotation.JsonInclude;
@@ -15,7 +18,7 @@
 import de.tum.in.www1.artemis.config.Constants;
 import de.tum.in.www1.artemis.domain.quiz.scoring.*;
 import de.tum.in.www1.artemis.domain.view.QuizView;
-import de.tum.in.www1.artemis.service.EntityFileService;
+import de.tum.in.www1.artemis.exception.FilePathParsingException;
 import de.tum.in.www1.artemis.service.FilePathService;
 import de.tum.in.www1.artemis.service.FileService;
 
@@ -28,16 +31,13 @@
 public class DragAndDropQuestion extends QuizQuestion {
 
     @Transient
-    private final transient FilePathService filePathService = new FilePathService();
+    private final transient Logger log = LoggerFactory.getLogger(DragAndDropQuestion.class);
 
     @Transient
-    private final transient FileService fileService = new FileService();
-
-    @Transient
-    private final transient EntityFileService entityFileService = new EntityFileService(fileService, filePathService);
+    private final transient FilePathService filePathService = new FilePathService();
 
     @Transient
-    private String prevBackgroundFilePath;
+    private final transient FileService fileService = new FileService();
 
     @Column(name = "background_file_path")
     @JsonView(QuizView.Before.class)
@@ -139,42 +139,23 @@ public Boolean isValid() {
             return false;
         }
 
+        // A drag item can either be a text or a picture, but not both or none
+        for (DragItem dragItem : dragItems) {
+            if (StringUtils.isEmpty(dragItem.getText()) == StringUtils.isEmpty(dragItem.getPictureFilePath())) {
+                return false;
+            }
+        }
+
         // check if at least one correct mapping exists
         return getCorrectMappings() != null && !getCorrectMappings().isEmpty();
 
-        // TODO (?): Add checks for "is solvable" and "no misleading correct mapping" --> look at the implementation in the client
+        // TODO: (?) Add checks for "is solvable" and "no misleading correct mapping" --> look at the implementation in the client
     }
 
-    /*
-     * NOTE: The file management is necessary to differentiate between temporary and used files and to delete used files when the corresponding question is deleted or it is
-     * replaced by another file. The workflow is as follows 1. user uploads a file -> this is a temporary file, because at this point the corresponding question might not exist
-     * yet. 2. user saves the question -> now we move the temporary file which is addressed in backgroundFilePath to a permanent location and update the value in backgroundFilePath
-     * accordingly. => This happens in @PrePersist and @PostPersist 3. user might upload another file to replace the existing file -> this new file is a temporary file at first 4.
-     * user saves changes (with the new backgroundFilePath pointing to the new temporary file) -> now we delete the old file in the permanent location and move the new file to a
-     * permanent location and update the value in backgroundFilePath accordingly. => This happens in @PreUpdate and uses @PostLoad to know the old path 5. When question is deleted,
-     * the file in the permanent location is deleted => This happens in @PostRemove
-     */
-
     /**
-     * Initialisation of the DragAndDropQuestion on Server start
+     * This method is called after the entity is saved for the first time. We replace the placeholder in the backgroundFilePath with the id of the entity because we don't know it
+     * before creation.
      */
-    @PostLoad
-    public void onLoad() {
-        // replace placeholder with actual id if necessary (this is needed because changes made in afterCreate() are not persisted)
-        if (backgroundFilePath != null && backgroundFilePath.contains(Constants.FILEPATH_ID_PLACEHOLDER)) {
-            backgroundFilePath = backgroundFilePath.replace(Constants.FILEPATH_ID_PLACEHOLDER, getId().toString());
-        }
-        // save current path as old path (needed to know old path in onUpdate() and onDelete())
-        prevBackgroundFilePath = backgroundFilePath;
-    }
-
-    @PrePersist
-    public void beforeCreate() {
-        if (backgroundFilePath != null) {
-            backgroundFilePath = entityFileService.moveTempFileBeforeEntityPersistence(backgroundFilePath, FilePathService.getDragAndDropBackgroundFilePath(), false);
-        }
-    }
-
     @PostPersist
     public void afterCreate() {
         // replace placeholder with actual id if necessary (id is no longer null at this point)
@@ -183,16 +164,20 @@ public void afterCreate() {
         }
     }
 
-    @PreUpdate
-    public void onUpdate() {
-        backgroundFilePath = entityFileService.handlePotentialFileUpdateBeforeEntityPersistence(getId(), prevBackgroundFilePath, backgroundFilePath,
-                FilePathService.getDragAndDropBackgroundFilePath(), false);
-    }
-
+    /**
+     * This method is called when deleting the entity. It makes sure that the corresponding file is deleted as well.
+     */
     @PostRemove
     public void onDelete() {
-        if (prevBackgroundFilePath != null) {
-            fileService.schedulePathForDeletion(Path.of(prevBackgroundFilePath), 0);
+        // delete old file if necessary
+        try {
+            if (backgroundFilePath != null) {
+                fileService.schedulePathForDeletion(filePathService.actualPathForPublicPathOrThrow(URI.create(backgroundFilePath)), 0);
+            }
+        }
+        catch (FilePathParsingException e) {
+            // if the file path is invalid, we don't need to delete it
+            log.warn("Could not delete file with path {}. Assume already deleted, entity can be removed.", backgroundFilePath, e);
         }
     }
 
@@ -219,7 +204,6 @@ public Set<DragItem> getCorrectDragItemsForDropLocation(DropLocation dropLocatio
      * @return the dragItem with the given ID, or null if the dragItem is not contained in this question
      */
     public DragItem findDragItemById(Long dragItemId) {
-
         if (dragItemId != null) {
             // iterate through all dragItems of this quiz
             for (DragItem dragItem : dragItems) {
@@ -239,7 +223,6 @@ public DragItem findDragItemById(Long dragItemId) {
      * @return the dropLocation with the given ID, or null if the dropLocation is not contained in this question
      */
     public DropLocation findDropLocationById(Long dropLocationId) {
-
         if (dropLocationId != null) {
             // iterate through all dropLocations of this quiz
             for (DropLocation dropLocation : dropLocations) {
@@ -259,6 +242,7 @@ public DropLocation findDropLocationById(Long dropLocationId) {
      */
     public void undoUnallowedChanges(QuizQuestion originalQuizQuestion) {
         if (originalQuizQuestion instanceof DragAndDropQuestion dndOriginalQuestion) {
+            backgroundFilePath = dndOriginalQuestion.getBackgroundFilePath();
             // undo unallowed dragItemChanges
             undoUnallowedDragItemChanges(dndOriginalQuestion);
             // undo unallowed dragItemChanges
@@ -272,7 +256,6 @@ public void undoUnallowedChanges(QuizQuestion originalQuizQuestion) {
      * @param originalQuestion the original DragAndDrop-object, which will be compared with this question
      */
     private void undoUnallowedDragItemChanges(DragAndDropQuestion originalQuestion) {
-
         // find added DragItems, which are not allowed to be added
         Set<DragItem> notAllowedAddedDragItems = new HashSet<>();
         // check every dragItem of the question
@@ -281,6 +264,7 @@ private void undoUnallowedDragItemChanges(DragAndDropQuestion originalQuestion)
             if (originalQuestion.getDragItems().contains(dragItem)) {
                 // find original dragItem
                 DragItem originalDragItem = originalQuestion.findDragItemById(dragItem.getId());
+
                 // correct invalid = null to invalid = false
                 if (dragItem.isInvalid() == null) {
                     dragItem.setInvalid(false);
@@ -303,7 +287,6 @@ private void undoUnallowedDragItemChanges(DragAndDropQuestion originalQuestion)
      * @param originalQuestion the original DragAndDrop-object, which will be compared with this question
      */
     private void undoUnallowedDropLocationChanges(DragAndDropQuestion originalQuestion) {
-
         // find added DropLocations, which are not allowed to be added
         Set<DropLocation> notAllowedAddedDropLocations = new HashSet<>();
         // check every dropLocation of the question
@@ -355,9 +338,7 @@ public void initializeStatistic() {
      * @return a boolean which is true if the dragItem-changes make an update necessary and false if not
      */
     private boolean checkDragItemsIfRecalculationIsNecessary(DragAndDropQuestion originalQuestion) {
-
         boolean updateNecessary = false;
-
         // check every dragItem of the question
         for (DragItem dragItem : this.getDragItems()) {
             // check if the dragItem were already in the originalQuizExercise
@@ -388,9 +369,7 @@ private boolean checkDragItemsIfRecalculationIsNecessary(DragAndDropQuestion ori
      * @return a boolean which is true if the dropLocation-changes make an update necessary and false if not
      */
     private boolean checkDropLocationsIfRecalculationIsNecessary(DragAndDropQuestion originalQuestion) {
-
         boolean updateNecessary = false;
-
         // check every dropLocation of the question
         for (DropLocation dropLocation : this.getDropLocations()) {
             // check if the dropLocation were already in the originalQuizExercise

src/main/java/de/tum/in/www1/artemis/domain/quiz/DragItem.java

@@ -1,13 +1,15 @@
 package de.tum.in.www1.artemis.domain.quiz;
 
-import java.nio.file.Path;
+import java.net.URI;
 import java.util.HashSet;
 import java.util.Set;
 
 import javax.persistence.*;
 
 import org.hibernate.annotations.Cache;
 import org.hibernate.annotations.CacheConcurrencyStrategy;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 import com.fasterxml.jackson.annotation.JsonIgnore;
 import com.fasterxml.jackson.annotation.JsonInclude;
@@ -16,7 +18,7 @@
 import de.tum.in.www1.artemis.config.Constants;
 import de.tum.in.www1.artemis.domain.TempIdObject;
 import de.tum.in.www1.artemis.domain.view.QuizView;
-import de.tum.in.www1.artemis.service.EntityFileService;
+import de.tum.in.www1.artemis.exception.FilePathParsingException;
 import de.tum.in.www1.artemis.service.FilePathService;
 import de.tum.in.www1.artemis.service.FileService;
 
@@ -30,16 +32,13 @@
 public class DragItem extends TempIdObject implements QuizQuestionComponent<DragAndDropQuestion> {
 
     @Transient
-    private final transient FilePathService filePathService = new FilePathService();
-
-    @Transient
-    private final transient FileService fileService = new FileService();
+    private final transient Logger log = LoggerFactory.getLogger(DragItem.class);
 
     @Transient
-    private final transient EntityFileService entityFileService = new EntityFileService(fileService, filePathService);
+    private final transient FilePathService filePathService = new FilePathService();
 
     @Transient
-    private String prevPictureFilePath;
+    private final transient FileService fileService = new FileService();
 
     @Column(name = "picture_file_path")
     @JsonView(QuizView.Before.class)
@@ -66,13 +65,13 @@ public String getPictureFilePath() {
         return pictureFilePath;
     }
 
-    public void setPictureFilePath(String pictureFilePath) {
+    public DragItem pictureFilePath(String pictureFilePath) {
         this.pictureFilePath = pictureFilePath;
+        return this;
     }
 
-    public DragItem pictureFilePath(String pictureFilePath) {
+    public void setPictureFilePath(String pictureFilePath) {
         this.pictureFilePath = pictureFilePath;
-        return this;
     }
 
     public String getText() {
@@ -120,37 +119,10 @@ public DragItem removeMappings(DragAndDropMapping mapping) {
         return this;
     }
 
-    /*
-     * NOTE: The file management is necessary to differentiate between temporary and used files and to delete used files when the corresponding drag item is deleted or it is
-     * replaced by another file. The workflow is as follows 1. user uploads a file -> this is a temporary file, because at this point the corresponding drag item might not exist
-     * yet. 2. user saves the drag item -> now we move the temporary file which is addressed in pictureFilePath to a permanent location and update the value in pictureFilePath
-     * accordingly. => This happens in @PrePersist and @PostPersist 3. user might upload another file to replace the existing file -> this new file is a temporary file at first 4.
-     * user saves changes (with the new pictureFilePath pointing to the new temporary file) -> now we delete the old file in the permanent location and move the new file to a
-     * permanent location and update the value in pictureFilePath accordingly. => This happens in @PreUpdate and uses @PostLoad to know the old path 5. When drag item is deleted,
-     * the file in the permanent location is deleted => This happens in @PostRemove NOTE: Number 3 and 4 are not possible for drag items with the current UI, but might be possible
-     * in the future and are implemented here to prevent unexpected behaviour when UI changes and to keep code similar to DragAndDropQuestion.java
-     */
-
     /**
-     * Initialisation of the DragItem on Server start
+     * This method is called after the entity is saved for the first time. We replace the placeholder in the pictureFilePath with the id of the entity because we don't know it
+     * before creation.
      */
-    @PostLoad
-    public void onLoad() {
-        // replace placeholder with actual id if necessary (this is needed because changes made in afterCreate() are not persisted)
-        if (pictureFilePath != null && pictureFilePath.contains(Constants.FILEPATH_ID_PLACEHOLDER)) {
-            pictureFilePath = pictureFilePath.replace(Constants.FILEPATH_ID_PLACEHOLDER, getId().toString());
-        }
-        // save current path as old path (needed to know old path in onUpdate() and onDelete())
-        prevPictureFilePath = pictureFilePath;
-    }
-
-    @PrePersist
-    public void beforeCreate() {
-        if (pictureFilePath != null) {
-            pictureFilePath = entityFileService.moveTempFileBeforeEntityPersistence(pictureFilePath, FilePathService.getDragItemFilePath(), false);
-        }
-    }
-
     @PostPersist
     public void afterCreate() {
         // replace placeholder with actual id if necessary (id is no longer null at this point)
@@ -159,16 +131,20 @@ public void afterCreate() {
         }
     }
 
-    @PreUpdate
-    public void onUpdate() {
-        pictureFilePath = entityFileService.handlePotentialFileUpdateBeforeEntityPersistence(getId(), prevPictureFilePath, pictureFilePath, FilePathService.getDragItemFilePath(),
-                false);
-    }
-
+    /**
+     * This method is called when deleting this entity. It makes sure that the corresponding file is deleted as well.
+     */
     @PostRemove
     public void onDelete() {
-        if (prevPictureFilePath != null) {
-            fileService.schedulePathForDeletion(Path.of(prevPictureFilePath), 0);
+        // delete old file if necessary
+        try {
+            if (pictureFilePath != null) {
+                fileService.schedulePathForDeletion(filePathService.actualPathForPublicPathOrThrow(URI.create(pictureFilePath)), 0);
+            }
+        }
+        catch (FilePathParsingException e) {
+            // if the file path is invalid, we don't need to delete it
+            log.warn("Could not delete file with path {}. Assume already deleted, entity can be removed.", pictureFilePath, e);
         }
     }
 

src/main/java/de/tum/in/www1/artemis/domain/quiz/QuizExercise.java

@@ -589,7 +589,7 @@ else if (batch != null && batch.isStarted()) {
     public void validateDates() {
         super.validateDates();
         quizBatches.forEach(quizBatch -> {
-            if (quizBatch.getStartTime() != null && quizBatch.getStartTime().isBefore(getReleaseDate())) {
+            if (quizBatch.getStartTime() != null && getReleaseDate() != null && quizBatch.getStartTime().isBefore(getReleaseDate())) {
                 throw new BadRequestAlertException("Start time must not be before release date!", getTitle(), "noValidDates");
             }
         });

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

@@ -64,6 +64,11 @@ default QuizExercise findByIdElseThrow(Long quizExerciseId) throws EntityNotFoun
         return findById(quizExerciseId).orElseThrow(() -> new EntityNotFoundException("Quiz Exercise", quizExerciseId));
     }
 
+    @NotNull
+    default QuizExercise findWithEagerQuestionsByIdOrElseThrow(Long quizExerciseId) {
+        return findWithEagerQuestionsById(quizExerciseId).orElseThrow(() -> new EntityNotFoundException("QuizExercise", quizExerciseId));
+    };
+
     /**
      * Get one quiz exercise
      *

src/main/java/de/tum/in/www1/artemis/service/AttachmentUnitService.java

@@ -158,7 +158,7 @@ private void handleFile(MultipartFile file, Attachment attachment, boolean keepF
      */
     private void evictCache(MultipartFile file, AttachmentUnit attachmentUnit) {
         if (file != null && !file.isEmpty()) {
-            this.cacheManager.getCache("files").evict(filePathService.actualPathForPublicPath(URI.create(attachmentUnit.getAttachment().getLink())).toString());
+            this.cacheManager.getCache("files").evict(filePathService.actualPathForPublicPathOrThrow(URI.create(attachmentUnit.getAttachment().getLink())).toString());
         }
     }
 

src/main/java/de/tum/in/www1/artemis/service/FileUploadSubmissionService.java

@@ -156,7 +156,7 @@ private URI storeFile(FileUploadSubmission fileUploadSubmission, StudentParticip
             fileUploadSubmission = fileUploadSubmissionRepository.save(fileUploadSubmission);
         }
         final Path savePath = saveFileForSubmission(file, fileUploadSubmission, exercise);
-        final URI newFilePath = filePathService.publicPathForActualPath(savePath, fileUploadSubmission.getId());
+        final URI newFilePath = filePathService.publicPathForActualPathOrThrow(savePath, fileUploadSubmission.getId());
 
         // We need to ensure that we can access the store file and the stored file is the same as was passed to us in the request
         final var storedFileHash = DigestUtils.md5Hex(Files.newInputStream(savePath));

src/main/java/de/tum/in/www1/artemis/service/LectureImportService.java

@@ -162,15 +162,15 @@ private Attachment cloneAttachment(final Attachment importedAttachment) {
         attachment.setVersion(importedAttachment.getVersion());
         attachment.setAttachmentType(importedAttachment.getAttachmentType());
 
-        Path oldPath = filePathService.actualPathForPublicPath(URI.create(importedAttachment.getLink()));
+        Path oldPath = filePathService.actualPathForPublicPathOrThrow(URI.create(importedAttachment.getLink()));
         Path tempPath = FilePathService.getTempFilePath().resolve(oldPath.getFileName());
 
         try {
             log.debug("Copying attachment file from {} to {}", oldPath, tempPath);
             FileUtils.copyFile(oldPath.toFile(), tempPath.toFile(), REPLACE_EXISTING);
 
             // File was copied to a temp directory and will be moved once we persist the attachment
-            attachment.setLink(filePathService.publicPathForActualPath(tempPath, null).toString());
+            attachment.setLink(filePathService.publicPathForActualPathOrThrow(tempPath, null).toString());
         }
         catch (IOException e) {
             log.error("Error while copying file", e);