diff --git a/ruleset.xml b/ruleset.xml index 502e1450195c..caa96b49676c 100644 --- a/ruleset.xml +++ b/ruleset.xml @@ -131,7 +131,10 @@ http://pmd.sourceforge.net/ruleset/2.0.0 "> - + + // It's okay to use short variable names in DTOs, e.g. "id" or "name" + .*/de/tum/in/www1/artemis/web/rest/dto/.* + diff --git a/src/main/java/de/tum/in/www1/artemis/web/rest/CourseResource.java b/src/main/java/de/tum/in/www1/artemis/web/rest/CourseResource.java index 193ec41ed027..53c7e4229abf 100644 --- a/src/main/java/de/tum/in/www1/artemis/web/rest/CourseResource.java +++ b/src/main/java/de/tum/in/www1/artemis/web/rest/CourseResource.java @@ -21,10 +21,7 @@ import org.springframework.data.domain.Page; import org.springframework.data.domain.PageImpl; import org.springframework.data.domain.PageRequest; -import org.springframework.http.HttpHeaders; -import org.springframework.http.HttpStatus; -import org.springframework.http.MediaType; -import org.springframework.http.ResponseEntity; +import org.springframework.http.*; import org.springframework.web.bind.annotation.*; import org.springframework.web.multipart.MultipartFile; import org.springframework.web.server.ResponseStatusException; @@ -761,7 +758,11 @@ public ResponseEntity downloadCourseArchive(@PathVariable Long courseI File zipFile = archive.toFile(); InputStreamResource resource = new InputStreamResource(new FileInputStream(zipFile)); - return ResponseEntity.ok().contentLength(zipFile.length()).contentType(MediaType.APPLICATION_OCTET_STREAM).header("filename", zipFile.getName()).body(resource); + ContentDisposition contentDisposition = ContentDisposition.builder("attachment").filename(zipFile.getName()).build(); + HttpHeaders headers = new HttpHeaders(); + headers.setContentDisposition(contentDisposition); + return ResponseEntity.ok().headers(headers).contentLength(zipFile.length()).contentType(MediaType.APPLICATION_OCTET_STREAM).header("filename", zipFile.getName()) + .body(resource); } /** diff --git a/src/main/java/de/tum/in/www1/artemis/web/rest/ExamResource.java b/src/main/java/de/tum/in/www1/artemis/web/rest/ExamResource.java index 9f42c7385907..d3cd5fedc05b 100644 --- a/src/main/java/de/tum/in/www1/artemis/web/rest/ExamResource.java +++ b/src/main/java/de/tum/in/www1/artemis/web/rest/ExamResource.java @@ -24,10 +24,7 @@ import org.springframework.core.io.Resource; import org.springframework.data.domain.Page; import org.springframework.data.domain.Pageable; -import org.springframework.http.HttpHeaders; -import org.springframework.http.HttpStatus; -import org.springframework.http.MediaType; -import org.springframework.http.ResponseEntity; +import org.springframework.http.*; import org.springframework.web.bind.annotation.*; import org.springframework.web.servlet.support.ServletUriComponentsBuilder; @@ -1150,8 +1147,12 @@ public ResponseEntity downloadExamArchive(@PathVariable Long courseId, Path archive = Path.of(examArchivesDirPath, exam.getExamArchivePath()); File zipFile = archive.toFile(); + ContentDisposition contentDisposition = ContentDisposition.builder("attachment").filename(zipFile.getName()).build(); + HttpHeaders headers = new HttpHeaders(); + headers.setContentDisposition(contentDisposition); InputStreamResource resource = new InputStreamResource(new FileInputStream(zipFile)); - return ResponseEntity.ok().contentLength(zipFile.length()).contentType(MediaType.APPLICATION_OCTET_STREAM).header("filename", zipFile.getName()).body(resource); + return ResponseEntity.ok().headers(headers).contentLength(zipFile.length()).contentType(MediaType.APPLICATION_OCTET_STREAM).header("filename", zipFile.getName()) + .body(resource); } /** diff --git a/src/main/webapp/app/course/manage/course-management.service.ts b/src/main/webapp/app/course/manage/course-management.service.ts index 0639d98c2711..a95f1a978c77 100644 --- a/src/main/webapp/app/course/manage/course-management.service.ts +++ b/src/main/webapp/app/course/manage/course-management.service.ts @@ -417,11 +417,9 @@ export class CourseManagementService { * if the archive does not exist. * @param courseId The id of the course */ - downloadCourseArchive(courseId: number): Observable> { - return this.http.get(`${this.resourceUrl}/${courseId}/download-archive`, { - observe: 'response', - responseType: 'blob', - }); + downloadCourseArchive(courseId: number): void { + const url = `${this.resourceUrl}/${courseId}/download-archive`; + window.open(url, '_blank'); } /** diff --git a/src/main/webapp/app/exam/manage/exam-management.service.ts b/src/main/webapp/app/exam/manage/exam-management.service.ts index fd05d95cb618..a145093dc20f 100644 --- a/src/main/webapp/app/exam/manage/exam-management.service.ts +++ b/src/main/webapp/app/exam/manage/exam-management.service.ts @@ -468,11 +468,9 @@ export class ExamManagementService { * @param courseId * @param examId The id of the exam */ - downloadExamArchive(courseId: number, examId: number): Observable> { - return this.http.get(`${this.resourceUrl}/${courseId}/exams/${examId}/download-archive`, { - observe: 'response', - responseType: 'blob', - }); + downloadExamArchive(courseId: number, examId: number): void { + const url = `${this.resourceUrl}/${courseId}/exams/${examId}/download-archive`; + window.open(url, '_blank'); } /** diff --git a/src/main/webapp/app/shared/components/course-exam-archive-button/course-exam-archive-button.component.ts b/src/main/webapp/app/shared/components/course-exam-archive-button/course-exam-archive-button.component.ts index f24bc13c9a73..02aecadc1187 100644 --- a/src/main/webapp/app/shared/components/course-exam-archive-button/course-exam-archive-button.component.ts +++ b/src/main/webapp/app/shared/components/course-exam-archive-button/course-exam-archive-button.component.ts @@ -1,4 +1,4 @@ -import { ChangeDetectorRef, Component, Input, OnDestroy, OnInit, TemplateRef, ViewChild } from '@angular/core'; +import { Component, Input, OnDestroy, OnInit, TemplateRef, ViewChild } from '@angular/core'; import { AlertService } from 'app/core/util/alert.service'; import { CourseManagementService } from 'app/course/manage/course-management.service'; import { JhiWebsocketService } from 'app/core/websocket/websocket.service'; @@ -9,7 +9,6 @@ import { ExamManagementService } from 'app/exam/manage/exam-management.service'; import { Course } from 'app/entities/course.model'; import { Exam } from 'app/entities/exam.model'; import dayjs from 'dayjs/esm'; -import { downloadZipFileFromResponse } from 'app/shared/util/download.util'; import { ButtonSize } from '../button.component'; import { ActionType } from 'app/shared/delete-dialog/delete-dialog.model'; import { Subject } from 'rxjs'; @@ -70,7 +69,6 @@ export class CourseExamArchiveButtonComponent implements OnInit, OnDestroy { private translateService: TranslateService, private modalService: NgbModal, private accountService: AccountService, - private changeDetectionRef: ChangeDetectorRef, ) {} ngOnInit() { @@ -127,13 +125,11 @@ export class CourseExamArchiveButtonComponent implements OnInit, OnDestroy { if (this.archiveMode === 'Exam' && this.exam) { this.examService.find(this.course.id!, this.exam.id!).subscribe((res) => { this.exam = res.body!; - this.changeDetectionRef.detectChanges(); this.displayDownloadArchiveButton = this.canDownloadArchive(); }); } else { this.courseService.find(this.course.id!).subscribe((res) => { this.course = res.body!; - this.changeDetectionRef.detectChanges(); this.displayDownloadArchiveButton = this.canDownloadArchive(); }); } @@ -181,8 +177,6 @@ export class CourseExamArchiveButtonComponent implements OnInit, OnDestroy { } if (result === 'archive' || !this.canDownloadArchive()) { this.archive(); - } else { - this.reloadCourseOrExam(); } }, () => {}, @@ -211,15 +205,9 @@ export class CourseExamArchiveButtonComponent implements OnInit, OnDestroy { downloadArchive() { if (this.archiveMode === 'Exam' && this.exam) { - this.examService.downloadExamArchive(this.course.id!, this.exam.id!).subscribe({ - next: (response) => downloadZipFileFromResponse(response), - error: () => this.alertService.error('artemisApp.courseExamArchive.archiveDownloadError'), - }); + this.examService.downloadExamArchive(this.course.id!, this.exam.id!); } else { - this.courseService.downloadCourseArchive(this.course.id!).subscribe({ - next: (response) => downloadZipFileFromResponse(response), - error: () => this.alertService.error('artemisApp.courseExamArchive.archiveDownloadError'), - }); + this.courseService.downloadCourseArchive(this.course.id!); } } diff --git a/src/test/java/de/tum/in/www1/artemis/course/CourseTestService.java b/src/test/java/de/tum/in/www1/artemis/course/CourseTestService.java index 8e547f2b3c99..6135065201d5 100644 --- a/src/test/java/de/tum/in/www1/artemis/course/CourseTestService.java +++ b/src/test/java/de/tum/in/www1/artemis/course/CourseTestService.java @@ -2176,9 +2176,11 @@ public void testDownloadCourseArchiveAsInstructor_not_found() throws Exception { public void testDownloadCourseArchiveAsInstructor() throws Exception { // Archive the course and wait until it's complete Course updatedCourse = testArchiveCourseWithTestModelingAndFileUploadExercises(); + Map expectedHeaders = new HashMap<>(); + expectedHeaders.put("Content-Disposition", "attachment; filename=\"" + updatedCourse.getCourseArchivePath() + "\""); // Download the archive - var archive = request.getFile("/api/courses/" + updatedCourse.getId() + "/download-archive", HttpStatus.OK, new LinkedMultiValueMap<>()); + var archive = request.getFile("/api/courses/" + updatedCourse.getId() + "/download-archive", HttpStatus.OK, new LinkedMultiValueMap<>(), expectedHeaders); assertThat(archive).isNotNull(); assertThat(archive).exists(); assertThat(archive.getPath().length()).isGreaterThanOrEqualTo(4); diff --git a/src/test/java/de/tum/in/www1/artemis/exam/ExamIntegrationTest.java b/src/test/java/de/tum/in/www1/artemis/exam/ExamIntegrationTest.java index f1016817f6aa..ae08e0c9a3c2 100644 --- a/src/test/java/de/tum/in/www1/artemis/exam/ExamIntegrationTest.java +++ b/src/test/java/de/tum/in/www1/artemis/exam/ExamIntegrationTest.java @@ -1069,7 +1069,10 @@ void testDownloadExamArchiveAsInstructor() throws Exception { // Download the archive var exam = examRepository.findByCourseId(course.getId()).stream().findFirst().orElseThrow(); - var archive = request.getFile("/api/courses/" + course.getId() + "/exams/" + exam.getId() + "/download-archive", HttpStatus.OK, new LinkedMultiValueMap<>()); + Map expectedHeaders = new HashMap<>(); + expectedHeaders.put("Content-Disposition", "attachment; filename=\"" + exam.getExamArchivePath() + "\""); + var archive = request.getFile("/api/courses/" + course.getId() + "/exams/" + exam.getId() + "/download-archive", HttpStatus.OK, new LinkedMultiValueMap<>(), + expectedHeaders); assertThat(archive).isNotNull(); // Extract the archive 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 551f2422811b..1f3fe7254ca9 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 @@ -69,9 +69,9 @@ public ObjectMapper getObjectMapper() { * @param file the optional file to be sent * @param responseType the expected response type as class * @param expectedStatus the expected status + * @param the type of the main object to send + * @param the type of the response object * @return the response as object of the given type or null if the status is not 2xx - * @param the type of the main object to send - * @param the type of the response object * @throws Exception if the request fails */ public R postWithMultipartFile(String path, T paramValue, String paramName, MockMultipartFile file, Class responseType, HttpStatus expectedStatus) throws Exception { @@ -87,9 +87,9 @@ public R postWithMultipartFile(String path, T paramValue, String paramNam * @param files the optional files to be sent * @param responseType the expected response type as class * @param expectedStatus the expected status + * @param the type of the main object to send + * @param the type of the response object * @return the response as object of the given type or null if the status is not 2xx - * @param the type of the main object to send - * @param the type of the response object * @throws Exception if the request fails */ public R postWithMultipartFiles(String path, T paramValue, String paramName, List files, Class responseType, HttpStatus expectedStatus) @@ -380,9 +380,9 @@ public R postWithResponseBody(String path, T body, Class responseType) * @param responseType the expected response type as class * @param expectedStatus the expected status * @param params the optional parameters for the request + * @param the type of the main object to send + * @param the type of the response object * @return the response as object of the given type or null if the status is not 2xx - * @param the type of the main object to send - * @param the type of the response object * @throws Exception if the request fails */ public R putWithMultipartFile(String path, T paramValue, String paramName, MockMultipartFile file, Class responseType, HttpStatus expectedStatus, @@ -400,9 +400,9 @@ public R putWithMultipartFile(String path, T paramValue, String paramName * @param responseType the expected response type as class * @param expectedStatus the expected status * @param params the optional parameters for the request + * @param the type of the main object to send + * @param the type of the response object * @return the response as object of the given type or null if the status is not 2xx - * @param the type of the main object to send - * @param the type of the response object * @throws Exception if the request fails */ public R putWithMultipartFiles(String path, T paramValue, String paramName, List files, Class responseType, HttpStatus expectedStatus, @@ -558,12 +558,17 @@ public T get(String path, HttpStatus expectedStatus, Class responseType, } public File getFile(String path, HttpStatus expectedStatus, MultiValueMap params) throws Exception { + return getFile(path, expectedStatus, params, null); + } + + public File getFile(String path, HttpStatus expectedStatus, MultiValueMap params, @Nullable Map expectedResponseHeaders) throws Exception { MvcResult res = mvc.perform(MockMvcRequestBuilders.get(new URI(path)).params(params).headers(new HttpHeaders())).andExpect(status().is(expectedStatus.value())).andReturn(); restoreSecurityContext(); if (!expectedStatus.is2xxSuccessful()) { assertThat(res.getResponse().containsHeader("location")).as("no location header on failed request").isFalse(); return null; } + verifyExpectedResponseHeaders(expectedResponseHeaders, res); String tmpDirectory = System.getProperty("java.io.tmpdir"); var filename = res.getResponse().getHeader("filename"); diff --git a/src/test/javascript/spec/component/course/course-management.service.spec.ts b/src/test/javascript/spec/component/course/course-management.service.spec.ts index 50bb53271755..61cacac53048 100644 --- a/src/test/javascript/spec/component/course/course-management.service.spec.ts +++ b/src/test/javascript/spec/component/course/course-management.service.spec.ts @@ -426,15 +426,11 @@ describe('Course Management Service', () => { tick(); })); - it('should download course archive', fakeAsync(() => { - const expectedBlob = new Blob(['abc', 'cfe']); - courseManagementService.downloadCourseArchive(course.id!).subscribe((resp) => { - expect(resp.body).toEqual(expectedBlob); - }); - const req = httpMock.expectOne({ method: 'GET', url: `${resourceUrl}/${course.id}/download-archive` }); - req.flush(expectedBlob); - tick(); - })); + it('should download course archive', () => { + const windowSpy = jest.spyOn(window, 'open').mockImplementation(); + courseManagementService.downloadCourseArchive(1); + expect(windowSpy).toHaveBeenCalledWith('api/courses/1/download-archive', '_blank'); + }); it('should archive the course', fakeAsync(() => { courseManagementService.archiveCourse(course.id!).subscribe((res) => expect(res.body).toEqual(course)); diff --git a/src/test/javascript/spec/component/exam/manage/exam-management.service.spec.ts b/src/test/javascript/spec/component/exam/manage/exam-management.service.spec.ts index 8370f024633e..9f1b6c9ecc84 100644 --- a/src/test/javascript/spec/component/exam/manage/exam-management.service.spec.ts +++ b/src/test/javascript/spec/component/exam/manage/exam-management.service.spec.ts @@ -641,21 +641,11 @@ describe('Exam Management Service Tests', () => { })); it('should download the exam from archive', fakeAsync(() => { - // GIVEN const mockExam: Exam = { id: 1 }; - const mockResponse = new Blob(); - const expected = new Blob(); - - // WHEN - service.downloadExamArchive(course.id!, mockExam.id!).subscribe((res) => expect(res.body).toEqual(expected)); - // THEN - const req = httpMock.expectOne({ - method: 'GET', - url: `${service.resourceUrl}/${course.id}/exams/${mockExam.id}/download-archive`, - }); - req.flush(mockResponse); - tick(); + const windowSpy = jest.spyOn(window, 'open').mockImplementation(); + service.downloadExamArchive(course.id!, mockExam.id!); + expect(windowSpy).toHaveBeenCalledWith('api/courses/456/exams/1/download-archive', '_blank'); })); it('should archive the exam', fakeAsync(() => { diff --git a/src/test/javascript/spec/component/shared/course-exam-archive-button.component.spec.ts b/src/test/javascript/spec/component/shared/course-exam-archive-button.component.spec.ts index 287f39430d54..2dfb1b17f5f6 100644 --- a/src/test/javascript/spec/component/shared/course-exam-archive-button.component.spec.ts +++ b/src/test/javascript/spec/component/shared/course-exam-archive-button.component.spec.ts @@ -170,8 +170,7 @@ describe('Course Exam Archive Button Component', () => { })); it('should download archive for course', fakeAsync(() => { - const response: HttpResponse = new HttpResponse({ status: 200 }); - const downloadStub = jest.spyOn(courseManagementService, 'downloadCourseArchive').mockReturnValue(of(response)); + const downloadStub = jest.spyOn(courseManagementService, 'downloadCourseArchive').mockImplementation(() => {}); comp.downloadArchive(); @@ -253,14 +252,13 @@ describe('Course Exam Archive Button Component', () => { expect(comp.canCleanupCourse()).toBeFalse(); })); - it('should download archive for exam', fakeAsync(() => { - const response: HttpResponse = new HttpResponse({ status: 200 }); - const downloadStub = jest.spyOn(examManagementService, 'downloadExamArchive').mockReturnValue(of(response)); + it('should download archive for exam', () => { + const downloadStub = jest.spyOn(examManagementService, 'downloadExamArchive').mockImplementation(() => {}); comp.downloadArchive(); expect(downloadStub).toHaveBeenCalledOnce(); - })); + }); it('should archive course', fakeAsync(() => { const response: HttpResponse = new HttpResponse({ status: 200 });