From a714635a4949bd3d9cd89e6ba7e994687c4c8a68 Mon Sep 17 00:00:00 2001 From: thilo-behnke Date: Mon, 23 Sep 2019 09:50:28 +0200 Subject: [PATCH] Bugfix/programming exercise/fix broken endpoints (#838) --- .../www1/artemis/service/ExerciseService.java | 3 +- .../service/ProgrammingExerciseService.java | 31 ++++- .../web/rest/ProgrammingExerciseResource.java | 34 ++--- .../programming-exercise-update.component.ts | 5 +- .../www1/artemis/ProgrammingExerciseTest.java | 122 ++++++++++++++++++ ...ingSubmissionAndResultIntegrationTest.java | 11 +- .../artemis/util/DatabaseUtilService.java | 5 +- .../www1/artemis/util/RequestUtilService.java | 12 ++ 8 files changed, 190 insertions(+), 33 deletions(-) create mode 100644 src/test/java/de/tum/in/www1/artemis/ProgrammingExerciseTest.java diff --git a/src/main/java/de/tum/in/www1/artemis/service/ExerciseService.java b/src/main/java/de/tum/in/www1/artemis/service/ExerciseService.java index 042767b7eeaa..a190d033c9d1 100644 --- a/src/main/java/de/tum/in/www1/artemis/service/ExerciseService.java +++ b/src/main/java/de/tum/in/www1/artemis/service/ExerciseService.java @@ -282,8 +282,7 @@ public void delete(Exercise exercise, boolean deleteStudentReposBuildPlans, bool participationService.deleteAllByExerciseId(exercise.getId(), deleteStudentReposBuildPlans, deleteStudentReposBuildPlans); // Programming exercises have some special stuff that needs to be cleaned up (solution/template participation, build plans, etc.). if (exercise instanceof ProgrammingExercise) { - ProgrammingExercise programmingExercise = (ProgrammingExercise) exercise; - programmingExerciseService.get().delete(programmingExercise, deleteBaseReposBuildPlans); + programmingExerciseService.get().delete(exercise.getId(), deleteBaseReposBuildPlans); } else { exerciseRepository.delete(exercise); diff --git a/src/main/java/de/tum/in/www1/artemis/service/ProgrammingExerciseService.java b/src/main/java/de/tum/in/www1/artemis/service/ProgrammingExerciseService.java index 7a6bfb0f6514..f5a0019d3010 100644 --- a/src/main/java/de/tum/in/www1/artemis/service/ProgrammingExerciseService.java +++ b/src/main/java/de/tum/in/www1/artemis/service/ProgrammingExerciseService.java @@ -626,6 +626,29 @@ public void squashAllCommitsOfRepositoryIntoOne(URL repoUrl) throws InterruptedE gitService.squashAllCommitsIntoInitialCommit(exerciseRepository); } + /** + * Updates the problem statement of the given programming exercise. + * + * @param programmingExerciseId ProgrammingExercise Id. + * @param problemStatement markdown of the problem statement. + * @return the updated ProgrammingExercise object. + * @throws EntityNotFoundException if there is no ProgrammingExercise for the given id. + * @throws IllegalAccessException if the user does not have permissions to access the ProgrammingExercise. + */ + public ProgrammingExercise updateProblemStatement(Long programmingExerciseId, String problemStatement) throws EntityNotFoundException, IllegalAccessException { + Optional programmingExerciseOpt = programmingExerciseRepository.findById(programmingExerciseId); + if (programmingExerciseOpt.isEmpty()) { + throw new EntityNotFoundException("Programming exercise not found with id: " + programmingExerciseId); + } + ProgrammingExercise programmingExercise = programmingExerciseOpt.get(); + User user = userService.getUserWithGroupsAndAuthorities(); + if (!authCheckService.isAtLeastInstructorForExercise(programmingExercise, user)) { + throw new IllegalAccessException("User with login " + user.getLogin() + " is not authorized to access programming exercise with id: " + programmingExerciseId); + } + programmingExercise.setProblemStatement(problemStatement); + return programmingExercise; + } + /** * This method calls the StructureOracleGenerator, generates the string out of the JSON representation of the structure oracle of the programming exercise and returns true if * the file was updated or generated, false otherwise. This can happen if the contents of the file have not changed. @@ -702,11 +725,13 @@ public boolean generateStructureOracleFile(URL solutionRepoURL, URL exerciseRepo /** * Delete a programming exercise, including its template and solution participations. * - * @param programmingExercise to delete. + * @param programmingExerciseId id of the programming exercise to delete. * @param deleteBaseReposBuildPlans if true will also delete build plans and projects. */ - @Transactional - public void delete(ProgrammingExercise programmingExercise, boolean deleteBaseReposBuildPlans) { + public void delete(Long programmingExerciseId, boolean deleteBaseReposBuildPlans) { + // TODO: This method does not accept a programming exercise to solve issues with nested Transactions. + // It would be good to refactor the delete calls and move the validity checks down from the resources to the service methods (e.g. EntityNotFound). + ProgrammingExercise programmingExercise = programmingExerciseRepository.findById(programmingExerciseId).get(); if (deleteBaseReposBuildPlans) { if (programmingExercise.getTemplateBuildPlanId() != null) { continuousIntegrationService.get().deleteBuildPlan(programmingExercise.getTemplateBuildPlanId()); diff --git a/src/main/java/de/tum/in/www1/artemis/web/rest/ProgrammingExerciseResource.java b/src/main/java/de/tum/in/www1/artemis/web/rest/ProgrammingExerciseResource.java index 6925c996d9a8..148928c74213 100644 --- a/src/main/java/de/tum/in/www1/artemis/web/rest/ProgrammingExerciseResource.java +++ b/src/main/java/de/tum/in/www1/artemis/web/rest/ProgrammingExerciseResource.java @@ -39,6 +39,7 @@ import de.tum.in.www1.artemis.service.connectors.ContinuousIntegrationService; import de.tum.in.www1.artemis.service.connectors.VersionControlService; import de.tum.in.www1.artemis.service.scheduled.ProgrammingExerciseScheduleService; +import de.tum.in.www1.artemis.web.rest.errors.EntityNotFoundException; import de.tum.in.www1.artemis.web.rest.util.HeaderUtil; import io.github.jhipster.web.util.ResponseUtil; @@ -341,39 +342,28 @@ public ResponseEntity updateProgrammingExercise(@RequestBod * * @param problemStatementUpdate the programmingExercise to update with the new problemStatement * @param notificationText to notify the student group about the updated problemStatement on the programming exercise - * @return the ResponseEntity with status 200 (OK) and with body the updated problemStatement, or with status 400 (Bad Request) if the programmingExercise is not valid, or with - * status 500 (Internal Server Error) if the programmingExercise couldn't be updated. - * @throws URISyntaxException if the Location URI syntax is incorrect + * @return the ResponseEntity with status 200 (OK) and with body the updated problemStatement, with status 404 if the programmingExercise could not be found, or with 403 if the user does not have permissions to access the programming exercise. */ @PatchMapping("/programming-exercises-problem") @PreAuthorize("hasAnyRole('TA', 'INSTRUCTOR', 'ADMIN')") public ResponseEntity updateProblemStatement(@RequestBody ProblemStatementUpdate problemStatementUpdate, - @RequestParam(value = "notificationText", required = false) String notificationText) throws URISyntaxException { + @RequestParam(value = "notificationText", required = false) String notificationText) { log.debug("REST request to update ProgrammingExercise with new problem statement: {}", problemStatementUpdate); - // fetch course from database to make sure client didn't change groups - ProgrammingExercise programmingExercise = (ProgrammingExercise) exerciseService.findOne(problemStatementUpdate.getExerciseId()); - Course course = courseService.findOne(programmingExercise.getCourse().getId()); - if (course == null) { - return ResponseEntity.badRequest() - .headers(HeaderUtil.createAlert(applicationName, "courseNotFound", "The course belonging to this programming exercise does not exist")).body(null); + ProgrammingExercise updatedProgrammingExercise; + try { + updatedProgrammingExercise = programmingExerciseService.updateProblemStatement(problemStatementUpdate.getExerciseId(), problemStatementUpdate.getProblemStatement()); } - User user = userService.getUserWithGroupsAndAuthorities(); - if (!authCheckService.isAtLeastTeachingAssistantInCourse(course, user)) { + catch (IllegalAccessException ex) { return forbidden(); } - - ResponseEntity errorResponse = checkProgrammingExerciseForError(programmingExercise); - if (errorResponse != null) { - return errorResponse; + catch (EntityNotFoundException ex) { + return notFound(); } - - programmingExercise.setProblemStatement(problemStatementUpdate.getProblemStatement()); - - ProgrammingExercise result = programmingExerciseRepository.save(programmingExercise); if (notificationText != null) { - groupNotificationService.notifyStudentGroupAboutExerciseUpdate(result, notificationText); + groupNotificationService.notifyStudentGroupAboutExerciseUpdate(updatedProgrammingExercise, notificationText); } - return ResponseEntity.ok().headers(HeaderUtil.createEntityUpdateAlert(applicationName, true, ENTITY_NAME, programmingExercise.getTitle())).body(result); + return ResponseEntity.ok().headers(HeaderUtil.createEntityUpdateAlert(applicationName, true, ENTITY_NAME, updatedProgrammingExercise.getTitle())) + .body(updatedProgrammingExercise); } /** diff --git a/src/main/webapp/app/entities/programming-exercise/programming-exercise-update.component.ts b/src/main/webapp/app/entities/programming-exercise/programming-exercise-update.component.ts index e2354eb136e3..6600b889b363 100644 --- a/src/main/webapp/app/entities/programming-exercise/programming-exercise-update.component.ts +++ b/src/main/webapp/app/entities/programming-exercise/programming-exercise-update.component.ts @@ -58,7 +58,10 @@ export class ProgrammingExerciseUpdateComponent implements OnInit { */ set selectedProgrammingLanguage(language: ProgrammingLanguage) { this.selectedProgrammingLanguageValue = language; - this.loadProgrammingLanguageTemplate(language); + // Don't override the problem statement with the template in edit mode. + if (this.programmingExercise.id === undefined) { + this.loadProgrammingLanguageTemplate(language); + } } get selectedProgrammingLanguage() { diff --git a/src/test/java/de/tum/in/www1/artemis/ProgrammingExerciseTest.java b/src/test/java/de/tum/in/www1/artemis/ProgrammingExerciseTest.java new file mode 100644 index 000000000000..ed7d4ac55489 --- /dev/null +++ b/src/test/java/de/tum/in/www1/artemis/ProgrammingExerciseTest.java @@ -0,0 +1,122 @@ +package de.tum.in.www1.artemis; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.when; + +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.test.autoconfigure.jdbc.AutoConfigureTestDatabase; +import org.springframework.boot.test.autoconfigure.web.servlet.AutoConfigureMockMvc; +import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.boot.test.mock.mockito.MockBean; +import org.springframework.http.HttpStatus; +import org.springframework.security.test.context.support.WithMockUser; +import org.springframework.test.context.ActiveProfiles; +import org.springframework.test.context.junit.jupiter.SpringExtension; + +import de.tum.in.www1.artemis.domain.ProgrammingExercise; +import de.tum.in.www1.artemis.repository.ProgrammingExerciseRepository; +import de.tum.in.www1.artemis.security.SecurityUtils; +import de.tum.in.www1.artemis.service.connectors.BambooService; +import de.tum.in.www1.artemis.service.connectors.BitbucketService; +import de.tum.in.www1.artemis.util.DatabaseUtilService; +import de.tum.in.www1.artemis.util.RequestUtilService; +import de.tum.in.www1.artemis.web.rest.ProblemStatementUpdate; + +@ExtendWith(SpringExtension.class) +@SpringBootTest +@AutoConfigureMockMvc +@AutoConfigureTestDatabase +@ActiveProfiles("artemis, bamboo") +class ProgrammingExerciseTest { + + @Autowired + DatabaseUtilService database; + + @Autowired + RequestUtilService request; + + @Autowired + ProgrammingExerciseRepository programmingExerciseRepository; + + @MockBean + BambooService continuousIntegrationService; + + @MockBean + BitbucketService versionControlService; + + Long programmingExerciseId; + + @BeforeEach + void init() { + database.addUsers(2, 2, 2); + database.addCourseWithOneProgrammingExercise(); + + programmingExerciseId = programmingExerciseRepository.findAll().get(0).getId(); + } + + @AfterEach + void tearDown() { + database.resetDatabase(); + } + + void updateProgrammingExercise(ProgrammingExercise programmingExercise, String newProblem, String newTitle) throws Exception { + programmingExercise.setProblemStatement(newProblem); + programmingExercise.setTitle(newTitle); + when(continuousIntegrationService.buildPlanIdIsValid(programmingExercise.getTemplateBuildPlanId())).thenReturn(true); + when(versionControlService.repositoryUrlIsValid(programmingExercise.getTemplateRepositoryUrlAsUrl())).thenReturn(true); + when(continuousIntegrationService.buildPlanIdIsValid(programmingExercise.getSolutionBuildPlanId())).thenReturn(true); + when(versionControlService.repositoryUrlIsValid(programmingExercise.getSolutionRepositoryUrlAsUrl())).thenReturn(true); + + ProgrammingExercise updatedProgrammingExercise = request.putWithResponseBody("/api/programming-exercises", programmingExercise, ProgrammingExercise.class, HttpStatus.OK); + + // The result from the put response should be updated with the new data. + assertThat(updatedProgrammingExercise.getProblemStatement()).isEqualTo(newProblem); + assertThat(updatedProgrammingExercise.getTitle()).isEqualTo(newTitle); + + SecurityUtils.setAuthorizationObject(); + // There should still be only 1 programming exercise. + assertThat(programmingExerciseRepository.count()).isEqualTo(1); + // The programming exercise in the db should also be updated. + ProgrammingExercise fromDb = programmingExerciseRepository.findById(programmingExercise.getId()).get(); + assertThat(fromDb.getProblemStatement()).isEqualTo(newProblem); + assertThat(fromDb.getTitle()).isEqualTo(newTitle); + } + + @Test + @WithMockUser(value = "instructor1", roles = "INSTRUCTOR") + void updateProgrammingExerciseOnce() throws Exception { + ProgrammingExercise programmingExercise = programmingExerciseRepository.findById(programmingExerciseId).get(); + updateProgrammingExercise(programmingExercise, "new problem 1", "new title 1"); + } + + @Test + @WithMockUser(value = "instructor1", roles = "INSTRUCTOR") + void updateProgrammingExerciseTwice() throws Exception { + ProgrammingExercise programmingExercise = programmingExerciseRepository.findById(programmingExerciseId).get(); + updateProgrammingExercise(programmingExercise, "new problem 1", "new title 1"); + updateProgrammingExercise(programmingExercise, "new problem 2", "new title 2"); + } + + @Test + @WithMockUser(value = "instructor1", roles = "INSTRUCTOR") + void updateProblemStatement() throws Exception { + String newProblem = "a new problem statement"; + ProgrammingExercise programmingExercise = programmingExerciseRepository.findById(programmingExerciseId).get(); + ProblemStatementUpdate problemStatementUpdate = new ProblemStatementUpdate(); + problemStatementUpdate.setExerciseId(programmingExerciseId); + problemStatementUpdate.setProblemStatement(newProblem); + ProgrammingExercise updatedProgrammingExercise = request.patchWithResponseBody("/api/programming-exercises-problem", problemStatementUpdate, ProgrammingExercise.class, + HttpStatus.OK); + + assertThat(updatedProgrammingExercise.getProblemStatement()).isEqualTo(newProblem); + + SecurityUtils.setAuthorizationObject(); + ProgrammingExercise fromDb = programmingExerciseRepository.findById(programmingExerciseId).get(); + assertThat(fromDb.getProblemStatement()).isEqualTo(newProblem); + } + +} diff --git a/src/test/java/de/tum/in/www1/artemis/ProgrammingSubmissionAndResultIntegrationTest.java b/src/test/java/de/tum/in/www1/artemis/ProgrammingSubmissionAndResultIntegrationTest.java index 266ac16fb040..0e16b93b4431 100644 --- a/src/test/java/de/tum/in/www1/artemis/ProgrammingSubmissionAndResultIntegrationTest.java +++ b/src/test/java/de/tum/in/www1/artemis/ProgrammingSubmissionAndResultIntegrationTest.java @@ -5,6 +5,7 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.Mockito.when; +import java.net.URL; import java.util.*; import java.util.stream.Collectors; import java.util.stream.Stream; @@ -313,8 +314,10 @@ void shouldCreateSubmissionForManualBuildRun(IntegrationTestParticipationType pa @EnumSource(IntegrationTestParticipationType.class) @WithMockUser(username = "instructor1", roles = "INSTRUCTOR") void shouldTriggerManualBuildRunForLastCommit(IntegrationTestParticipationType participationType) throws Exception { + Long participationId = getParticipationIdByType(participationType, 0); ObjectId objectId = ObjectId.fromString("9b3a9bd71a0d80e5bbc42204c319ed3d1d4f0d6d"); - when(gitServiceMock.getLastCommitHash(null)).thenReturn(objectId); + URL repositoryUrl = ((ProgrammingExerciseParticipation) participationRepository.findById(participationId).get()).getRepositoryUrlAsUrl(); + when(gitServiceMock.getLastCommitHash(repositoryUrl)).thenReturn(objectId); triggerBuild(participationType, 0, HttpStatus.OK); // Now a submission for the manual build should exist. @@ -325,7 +328,6 @@ void shouldTriggerManualBuildRunForLastCommit(IntegrationTestParticipationType p assertThat(submission.getType()).isEqualTo(SubmissionType.MANUAL); assertThat(submission.isSubmitted()).isTrue(); - Long participationId = getParticipationIdByType(participationType, 0); SecurityUtils.setAuthorizationObject(); Participation participation = participationRepository.getOneWithEagerSubmissions(participationId); @@ -349,8 +351,10 @@ void shouldTriggerManualBuildRunForLastCommit(IntegrationTestParticipationType p @EnumSource(IntegrationTestParticipationType.class) @WithMockUser(username = "instructor1", roles = "INSTRUCTOR") void shouldTriggerInstructorBuildRunForLastCommit(IntegrationTestParticipationType participationType) throws Exception { + Long participationId = getParticipationIdByType(participationType, 0); + URL repositoryUrl = ((ProgrammingExerciseParticipation) participationRepository.findById(participationId).get()).getRepositoryUrlAsUrl(); ObjectId objectId = ObjectId.fromString("9b3a9bd71a0d80e5bbc42204c319ed3d1d4f0d6d"); - when(gitServiceMock.getLastCommitHash(null)).thenReturn(objectId); + when(gitServiceMock.getLastCommitHash(repositoryUrl)).thenReturn(objectId); triggerInstructorBuild(participationType, 0, HttpStatus.OK); // Now a submission for the manual build should exist. @@ -361,7 +365,6 @@ void shouldTriggerInstructorBuildRunForLastCommit(IntegrationTestParticipationTy assertThat(submission.getType()).isEqualTo(SubmissionType.INSTRUCTOR); assertThat(submission.isSubmitted()).isTrue(); - Long participationId = getParticipationIdByType(participationType, 0); SecurityUtils.setAuthorizationObject(); Participation participation = participationRepository.getOneWithEagerSubmissions(participationId); diff --git a/src/test/java/de/tum/in/www1/artemis/util/DatabaseUtilService.java b/src/test/java/de/tum/in/www1/artemis/util/DatabaseUtilService.java index bc491690d5c1..b90e0c87afda 100644 --- a/src/test/java/de/tum/in/www1/artemis/util/DatabaseUtilService.java +++ b/src/test/java/de/tum/in/www1/artemis/util/DatabaseUtilService.java @@ -205,6 +205,7 @@ public ProgrammingExercise addTemplateParticipationForProgrammingExercise(Progra participation.setProgrammingExercise(exercise); participation.setBuildPlanId("TEST201904BPROGRAMMINGEXERCISE6-BASE"); participation.setInitializationState(InitializationState.INITIALIZED); + participation.setRepositoryUrl("http://url/scm/TEST234454TEST234565/template.git"); templateProgrammingExerciseParticipationRepo.save(participation); exercise.setTemplateParticipation(participation); return programmingExerciseRepository.save(exercise); @@ -216,6 +217,7 @@ public ProgrammingExercise addSolutionParticipationForProgrammingExercise(Progra participation.setProgrammingExercise(exercise); participation.setBuildPlanId("TEST201904BPROGRAMMINGEXERCISE6-SOLUTION"); participation.setInitializationState(InitializationState.INITIALIZED); + participation.setRepositoryUrl("http://url/scm/TEST234454TEST234565/solution.git"); solutionProgrammingExerciseParticipationRepo.save(participation); exercise.setSolutionParticipation(participation); return programmingExerciseRepository.save(exercise); @@ -322,7 +324,8 @@ public void addCourseWithDifferentModelingExercises() { public Course addCourseWithOneProgrammingExercise() { Course course = ModelFactory.generateCourse(null, pastTimestamp, futureFutureTimestamp, new HashSet<>(), "tumuser", "tutor", "instructor"); - ProgrammingExercise programmingExercise = (ProgrammingExercise) new ProgrammingExercise().programmingLanguage(ProgrammingLanguage.JAVA).course(course); + ProgrammingExercise programmingExercise = (ProgrammingExercise) new ProgrammingExercise().programmingLanguage(ProgrammingLanguage.JAVA).course(course) + .title("programming exercise"); courseRepo.save(course); programmingExerciseRepository.save(programmingExercise); course.addExercises(programmingExercise); diff --git a/src/test/java/de/tum/in/www1/artemis/util/RequestUtilService.java b/src/test/java/de/tum/in/www1/artemis/util/RequestUtilService.java index d42b33c93b8b..9aed2df415c0 100644 --- a/src/test/java/de/tum/in/www1/artemis/util/RequestUtilService.java +++ b/src/test/java/de/tum/in/www1/artemis/util/RequestUtilService.java @@ -91,6 +91,18 @@ public R putWithResponseBody(String path, T body, Class responseType, return mapper.readValue(res.getResponse().getContentAsString(), responseType); } + public R patchWithResponseBody(String path, T body, Class responseType, HttpStatus expectedStatus) throws Exception { + String jsonBody = mapper.writeValueAsString(body); + MvcResult res = mvc.perform(MockMvcRequestBuilders.patch(new URI(path)).contentType(MediaType.APPLICATION_JSON).content(jsonBody).with(csrf())) + .andExpect(status().is(expectedStatus.value())).andReturn(); + + if (res.getResponse().getStatus() >= 299) { + return null; + } + + return mapper.readValue(res.getResponse().getContentAsString(), responseType); + } + public List putWithResponseBodyList(String path, T body, Class listElementType, HttpStatus expectedStatus) throws Exception { String jsonBody = mapper.writeValueAsString(body); MvcResult res = mvc.perform(MockMvcRequestBuilders.put(new URI(path)).contentType(MediaType.APPLICATION_JSON).content(jsonBody).with(csrf()))