code review comments and test improvement

ls1intum · Nov 20, 2023 · cec9057 · cec9057
1 parent 5fc19ca
commit cec9057
Show file tree

Hide file tree

Showing 6 changed files with 79 additions and 47 deletions.
diff --git a/src/main/java/de/tum/in/www1/artemis/repository/BuildPlanRepository.java b/src/main/java/de/tum/in/www1/artemis/repository/BuildPlanRepository.java
@@ -23,6 +23,15 @@ public interface BuildPlanRepository extends JpaRepository<BuildPlan, Long> {
             """)
     Optional<BuildPlan> findByProgrammingExercises_IdWithProgrammingExercises(@Param("exerciseId") long exerciseId);
 
+    @Query("""
+
+            SELECT buildPlan
+            FROM BuildPlan buildPlan
+                JOIN buildPlan.programmingExercises programmingExercises
+            WHERE programmingExercises.id = :exerciseId
+                """)
+    Optional<BuildPlan> findByProgrammingExercises_Id(long exerciseId);
+
     default BuildPlan findByProgrammingExercises_IdWithProgrammingExercisesElseThrow(final long exerciseId) {
         return findByProgrammingExercises_IdWithProgrammingExercises(exerciseId)
                 .orElseThrow(() -> new EntityNotFoundException("Could not find a build plan for exercise " + exerciseId));

diff --git a/src/main/java/de/tum/in/www1/artemis/service/export/ProgrammingExerciseExportService.java b/src/main/java/de/tum/in/www1/artemis/service/export/ProgrammingExerciseExportService.java
@@ -6,16 +6,12 @@
 import java.io.File;
 import java.io.IOException;
 import java.io.UncheckedIOException;
+import java.nio.charset.StandardCharsets;
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.time.ZonedDateTime;
 import java.time.format.DateTimeFormatter;
-import java.util.ArrayList;
-import java.util.Collections;
-import java.util.Comparator;
-import java.util.List;
-import java.util.Objects;
-import java.util.Optional;
+import java.util.*;
 import java.util.concurrent.CompletableFuture;
 import java.util.concurrent.Executors;
 import java.util.function.Predicate;
@@ -31,10 +27,7 @@
 import javax.xml.transform.TransformerFactory;
 import javax.xml.transform.dom.DOMSource;
 import javax.xml.transform.stream.StreamResult;
-import javax.xml.xpath.XPath;
-import javax.xml.xpath.XPathConstants;
-import javax.xml.xpath.XPathException;
-import javax.xml.xpath.XPathFactory;
+import javax.xml.xpath.*;
 
 import org.apache.commons.io.FileUtils;
 import org.eclipse.jgit.api.errors.GitAPIException;
@@ -48,8 +41,6 @@
 import org.xml.sax.InputSource;
 import org.xml.sax.SAXException;
 
-import com.google.common.base.Charsets;
-
 import de.tum.in.www1.artemis.domain.*;
 import de.tum.in.www1.artemis.domain.enumeration.ProgrammingLanguage;
 import de.tum.in.www1.artemis.domain.enumeration.RepositoryType;
@@ -149,10 +140,10 @@ private Path exportProgrammingExerciseMaterialWithStudentReposOptional(Programmi
             });
 
             // Export the build plan of a programming exercise, if one exists. Only relevant for Gitlab/Jenkins or Gitlab/GitlabCI setups.
-            var buildPlan = buildPlanRepository.findByProgrammingExercises_IdWithProgrammingExercises(exercise.getId());
+            var buildPlan = buildPlanRepository.findByProgrammingExercises_Id(exercise.getId());
             if (buildPlan.isPresent()) {
                 Path buildPlanPath = exportDir.orElseThrow().resolve(BUILD_PLAN_FILE_NAME);
-                FileUtils.writeStringToFile(buildPlanPath.toFile(), buildPlan.orElseThrow().getBuildPlan(), Charsets.UTF_8);
+                FileUtils.writeStringToFile(buildPlanPath.toFile(), buildPlan.orElseThrow().getBuildPlan(), StandardCharsets.UTF_8);
                 pathsToBeZipped.add(buildPlanPath);
             }
 

diff --git a/.../de/tum/in/www1/artemis/service/programming/ProgrammingExerciseImportFromFileService.java b/.../de/tum/in/www1/artemis/service/programming/ProgrammingExerciseImportFromFileService.java
@@ -4,6 +4,7 @@
 
 import java.io.IOException;
 import java.net.URISyntaxException;
+import java.nio.charset.StandardCharsets;
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.util.List;
@@ -22,20 +23,18 @@
 
 import com.fasterxml.jackson.databind.DeserializationFeature;
 import com.fasterxml.jackson.databind.ObjectMapper;
-import com.google.common.base.Charsets;
 
 import de.tum.in.www1.artemis.domain.*;
 import de.tum.in.www1.artemis.domain.enumeration.RepositoryType;
 import de.tum.in.www1.artemis.repository.BuildPlanRepository;
 import de.tum.in.www1.artemis.service.*;
 import de.tum.in.www1.artemis.service.connectors.GitService;
-import de.tum.in.www1.artemis.service.export.ProgrammingExerciseExportService;
 import de.tum.in.www1.artemis.web.rest.errors.BadRequestAlertException;
 
 @Service
 public class ProgrammingExerciseImportFromFileService {
 
-    private final Logger log = LoggerFactory.getLogger(ProgrammingExerciseExportService.class);
+    private final Logger log = LoggerFactory.getLogger(ProgrammingExerciseImportFromFileService.class);
 
     private final ProgrammingExerciseService programmingExerciseService;
 
@@ -126,7 +125,7 @@ private void importBuildPlanIfExisting(ProgrammingExercise programmingExercise,
         Path buildPlanPath = importExerciseDir.resolve(BUILD_PLAN_FILE_NAME);
         if (Files.exists(buildPlanPath)) {
             try {
-                buildPlanRepository.setBuildPlanForExercise(FileUtils.readFileToString(buildPlanPath.toFile(), Charsets.UTF_8), programmingExercise);
+                buildPlanRepository.setBuildPlanForExercise(FileUtils.readFileToString(buildPlanPath.toFile(), StandardCharsets.UTF_8), programmingExercise);
             }
             catch (IOException e) {
                 log.warn("Could not read build plan file. Continue importing the exercise but skipping the build plan.", e);

diff --git a/...temis/exercise/programmingexercise/ProgrammingExerciseBitbucketBambooIntegrationTest.java b/...temis/exercise/programmingexercise/ProgrammingExerciseBitbucketBambooIntegrationTest.java
@@ -349,7 +349,7 @@ void exportInstructorRepositories_forbidden() throws Exception {
     @Test
     @WithMockUser(username = TEST_PREFIX + "instructor1", roles = "INSTRUCTOR")
     void exportProgrammingExerciseInstructorMaterial() throws Exception {
-        programmingExerciseTestService.exportProgrammingExerciseInstructorMaterial_shouldReturnFile(true);
+        programmingExerciseTestService.exportProgrammingExerciseInstructorMaterial_shouldReturnFileWithoutBuildplan(true);
         // we have a working directory and one directory for each repository
         verify(fileService, times(4)).scheduleDirectoryPathForRecursiveDeletion(any(Path.class), eq(5L));
         verify(fileService).schedulePathForDeletion(any(Path.class), eq(5L));
@@ -435,29 +435,29 @@ void testArchiveCourseWithProgrammingExercise() throws Exception {
     @WithMockUser(username = TEST_PREFIX + "instructor1", roles = "INSTRUCTOR")
     void testExportProgrammingExerciseInstructorMaterial_failToCreateZip() throws Exception {
         doThrow(IOException.class).when(zipFileService).createZipFile(any(Path.class), any());
-        programmingExerciseTestService.exportProgrammingExerciseInstructorMaterial(HttpStatus.INTERNAL_SERVER_ERROR, true, true, true);
+        programmingExerciseTestService.exportProgrammingExerciseInstructorMaterial(HttpStatus.INTERNAL_SERVER_ERROR, true, true, true, false);
     }
 
     @Test
     @WithMockUser(username = TEST_PREFIX + "instructor1", roles = "INSTRUCTOR")
     void testExportProgrammingExerciseInstructorMaterial_failToCreateTempDir() throws Exception {
         try (MockedStatic<Files> mockedFiles = mockStatic(Files.class)) {
             mockedFiles.when(() -> Files.createTempDirectory(any(Path.class), any(String.class))).thenThrow(IOException.class);
-            programmingExerciseTestService.exportProgrammingExerciseInstructorMaterial(HttpStatus.INTERNAL_SERVER_ERROR, true, false, false);
+            programmingExerciseTestService.exportProgrammingExerciseInstructorMaterial(HttpStatus.INTERNAL_SERVER_ERROR, true, false, false, false);
         }
     }
 
     @Test
     @WithMockUser(username = TEST_PREFIX + "instructor1", roles = "INSTRUCTOR")
     void testExportProgrammingExerciseInstructorMaterial_embeddedFilesDontExist() throws Exception {
-        programmingExerciseTestService.exportProgrammingExerciseInstructorMaterial_shouldReturnFile(false);
+        programmingExerciseTestService.exportProgrammingExerciseInstructorMaterial_shouldReturnFileWithoutBuildplan(false);
     }
 
     @Test
     @WithMockUser(username = TEST_PREFIX + "instructor1", roles = "INSTRUCTOR")
     void testExportProgrammingExerciseInstructorMaterial_failToExportRepository() throws Exception {
         doThrow(GitException.class).when(fileService).getTemporaryUniquePathWithoutPathCreation(any(Path.class), anyLong());
-        programmingExerciseTestService.exportProgrammingExerciseInstructorMaterial(HttpStatus.INTERNAL_SERVER_ERROR, false, true, true);
+        programmingExerciseTestService.exportProgrammingExerciseInstructorMaterial(HttpStatus.INTERNAL_SERVER_ERROR, false, true, true, false);
     }
 
     @Test

diff --git a/...artemis/exercise/programmingexercise/ProgrammingExerciseGitlabJenkinsIntegrationTest.java b/...artemis/exercise/programmingexercise/ProgrammingExerciseGitlabJenkinsIntegrationTest.java
@@ -404,7 +404,7 @@ void exportInstructorRepositories_forbidden() throws Exception {
     @Test
     @WithMockUser(username = TEST_PREFIX + "instructor1", roles = "INSTRUCTOR")
     void exportProgrammingExerciseInstructorMaterial() throws Exception {
-        programmingExerciseTestService.exportProgrammingExerciseInstructorMaterial_shouldReturnFile(true);
+        programmingExerciseTestService.exportProgrammingExerciseInstructorMaterial_shouldReturnFileWithBuildplan();
         // we have a working directory and one directory for each repository
         verify(fileService, times(4)).scheduleDirectoryPathForRecursiveDeletion(any(Path.class), eq(5L));
         verify(fileService).schedulePathForDeletion(any(Path.class), eq(5L));
@@ -427,14 +427,14 @@ void exportProgrammingExerciseInstructorMaterial_problemStatementShouldContainTe
     void testExportProgrammingExerciseInstructorMaterial_failToCreateTempDir() throws Exception {
         try (MockedStatic<Files> mockedFiles = mockStatic(Files.class)) {
             mockedFiles.when(() -> Files.createTempDirectory(any(Path.class), any(String.class))).thenThrow(IOException.class);
-            programmingExerciseTestService.exportProgrammingExerciseInstructorMaterial(HttpStatus.INTERNAL_SERVER_ERROR, true, false, false);
+            programmingExerciseTestService.exportProgrammingExerciseInstructorMaterial(HttpStatus.INTERNAL_SERVER_ERROR, true, false, false, false);
         }
     }
 
     @Test
     @WithMockUser(username = TEST_PREFIX + "instructor1", roles = "INSTRUCTOR")
     void testExportProgrammingExerciseInstructorMaterial_embeddedFilesDontExist() throws Exception {
-        programmingExerciseTestService.exportProgrammingExerciseInstructorMaterial_shouldReturnFile(false);
+        programmingExerciseTestService.exportProgrammingExerciseInstructorMaterial_shouldReturnFile(false, false);
     }
 
     @Test