From d3b09321fc2ac1ddaf0534d683351746d56d643b Mon Sep 17 00:00:00 2001 From: entholzer Date: Thu, 28 Nov 2024 22:15:26 +0100 Subject: [PATCH 1/4] fix file downloads --- .../aet/artemis/core/web/FileResource.java | 22 ++++++++-------- .../lecture-attachments.component.html | 2 +- .../lecture/lecture-attachments.component.ts | 4 +-- .../attachment-unit.component.ts | 2 +- .../course-lecture-details.component.html | 2 +- .../course-lecture-details.component.ts | 6 ++--- .../webapp/app/shared/http/file.service.ts | 25 +++++++++++++------ 7 files changed, 36 insertions(+), 27 deletions(-) diff --git a/src/main/java/de/tum/cit/aet/artemis/core/web/FileResource.java b/src/main/java/de/tum/cit/aet/artemis/core/web/FileResource.java index 27332e5343a7..11c5d1278609 100644 --- a/src/main/java/de/tum/cit/aet/artemis/core/web/FileResource.java +++ b/src/main/java/de/tum/cit/aet/artemis/core/web/FileResource.java @@ -1,6 +1,8 @@ package de.tum.cit.aet.artemis.core.web; import static de.tum.cit.aet.artemis.core.config.Constants.PROFILE_CORE; +import static org.apache.velocity.shaded.commons.io.FilenameUtils.getBaseName; +import static org.apache.velocity.shaded.commons.io.FilenameUtils.getExtension; import java.io.IOException; import java.net.FileNameMap; @@ -409,20 +411,18 @@ public ResponseEntity getExamUserImage(@PathVariable Long examUserId) { /** * GET /files/attachments/lecture/:lectureId/:filename : Get the lecture attachment * - * @param lectureId ID of the lecture, the attachment belongs to - * @param filename the filename of the file + * @param lectureId ID of the lecture, the attachment belongs to + * @param attachmentName the filename of the file * @return The requested file, 403 if the logged-in user is not allowed to access it, or 404 if the file doesn't exist */ - @GetMapping("files/attachments/lecture/{lectureId}/{filename}") + @GetMapping("files/attachments/lecture/{lectureId}/{attachmentName}") @EnforceAtLeastStudent - public ResponseEntity getLectureAttachment(@PathVariable Long lectureId, @PathVariable String filename) { - log.debug("REST request to get lecture attachment : {}", filename); - String fileNameWithoutSpaces = filename.replaceAll(" ", "_"); - sanitizeFilenameElseThrow(fileNameWithoutSpaces); + public ResponseEntity getLectureAttachment(@PathVariable Long lectureId, @PathVariable String attachmentName) { + log.debug("REST request to get lecture attachment : {}", attachmentName); List lectureAttachments = attachmentRepository.findAllByLectureId(lectureId); - Attachment attachment = lectureAttachments.stream().filter(lectureAttachment -> lectureAttachment.getLink().endsWith(fileNameWithoutSpaces)).findAny() - .orElseThrow(() -> new EntityNotFoundException("Attachment", filename)); + Attachment attachment = lectureAttachments.stream().filter(lectureAttachment -> lectureAttachment.getName().equals(getBaseName(attachmentName))).findAny() + .orElseThrow(() -> new EntityNotFoundException("Attachment", attachmentName)); // get the course for a lecture attachment Lecture lecture = attachment.getLecture(); @@ -431,7 +431,7 @@ public ResponseEntity getLectureAttachment(@PathVariable Long lectureId, // check if the user is authorized to access the requested attachment unit checkAttachmentAuthorizationOrThrow(course, attachment); - return buildFileResponse(getActualPathFromPublicPathString(attachment.getLink()), Optional.of(attachment.getName())); + return buildFileResponse(getActualPathFromPublicPathString(attachment.getLink()), Optional.of(attachmentName)); } /** @@ -487,7 +487,7 @@ public ResponseEntity getAttachmentUnitAttachment(@PathVariable Long att // check if the user is authorized to access the requested attachment unit checkAttachmentAuthorizationOrThrow(course, attachment); - return buildFileResponse(getActualPathFromPublicPathString(attachment.getLink()), Optional.of(attachment.getName())); + return buildFileResponse(getActualPathFromPublicPathString(attachment.getLink()), Optional.of(attachment.getName() + "." + getExtension(attachment.getLink()))); } /** diff --git a/src/main/webapp/app/lecture/lecture-attachments.component.html b/src/main/webapp/app/lecture/lecture-attachments.component.html index 6192f270d64e..6ed7e3ed3ea8 100644 --- a/src/main/webapp/app/lecture/lecture-attachments.component.html +++ b/src/main/webapp/app/lecture/lecture-attachments.component.html @@ -61,7 +61,7 @@

{{ attachment.attachmentType }} @if (!isDownloadingAttachmentLink) { - + {{ attachment.name }} } diff --git a/src/main/webapp/app/lecture/lecture-attachments.component.ts b/src/main/webapp/app/lecture/lecture-attachments.component.ts index 262bbfe593a5..ece1cd67473b 100644 --- a/src/main/webapp/app/lecture/lecture-attachments.component.ts +++ b/src/main/webapp/app/lecture/lecture-attachments.component.ts @@ -200,10 +200,10 @@ export class LectureAttachmentsComponent implements OnDestroy { return item.id; } - downloadAttachment(downloadUrl: string): void { + downloadAttachment(downloadName: string, downloadUrl: string): void { if (!this.isDownloadingAttachmentLink) { this.isDownloadingAttachmentLink = downloadUrl; - this.fileService.downloadFile(this.fileService.replaceLectureAttachmentPrefixAndUnderscores(downloadUrl)); + this.fileService.downloadFileByAttachmentName(downloadUrl, downloadName); this.isDownloadingAttachmentLink = undefined; } } diff --git a/src/main/webapp/app/overview/course-lectures/attachment-unit/attachment-unit.component.ts b/src/main/webapp/app/overview/course-lectures/attachment-unit/attachment-unit.component.ts index 5634aeda7a5e..4d70111dc9c9 100644 --- a/src/main/webapp/app/overview/course-lectures/attachment-unit/attachment-unit.component.ts +++ b/src/main/webapp/app/overview/course-lectures/attachment-unit/attachment-unit.component.ts @@ -51,7 +51,7 @@ export class AttachmentUnitComponent extends LectureUnitDirective
  • @if (!isDownloadingLink) { - + {{ attachment.name }} } diff --git a/src/main/webapp/app/overview/course-lectures/course-lecture-details.component.ts b/src/main/webapp/app/overview/course-lectures/course-lecture-details.component.ts index 28f73df62dc5..45a2525a5554 100644 --- a/src/main/webapp/app/overview/course-lectures/course-lecture-details.component.ts +++ b/src/main/webapp/app/overview/course-lectures/course-lecture-details.component.ts @@ -128,10 +128,10 @@ export class CourseLectureDetailsComponent extends AbstractScienceComponent impl return attachment.link.split('.').pop()!; } - downloadAttachment(downloadUrl?: string): void { - if (!this.isDownloadingLink && downloadUrl) { + downloadAttachment(downloadUrl?: string, donwloadName?: string): void { + if (!this.isDownloadingLink && downloadUrl && donwloadName) { this.isDownloadingLink = downloadUrl; - this.fileService.downloadFile(this.fileService.replaceLectureAttachmentPrefixAndUnderscores(downloadUrl)); + this.fileService.downloadFileByAttachmentName(downloadUrl, donwloadName); this.isDownloadingLink = undefined; } } diff --git a/src/main/webapp/app/shared/http/file.service.ts b/src/main/webapp/app/shared/http/file.service.ts index 05960940f7aa..1c9f264f41d3 100644 --- a/src/main/webapp/app/shared/http/file.service.ts +++ b/src/main/webapp/app/shared/http/file.service.ts @@ -82,6 +82,23 @@ export class FileService { return newWindow; } + /** + * Downloads the file from the provided downloadUrl and the attachment name + * + * @param downloadUrl url that is stored in the attachment model + * @param downloadName the name given to the attachment + */ + downloadFileByAttachmentName(downloadUrl: string, downloadName: string) { + const downloadUrlComponents = downloadUrl.split('/'); + // take the last element + const extension = downloadUrlComponents.pop()!.split('.').pop(); + const restOfUrl = downloadUrlComponents.join('/'); + const normalizedDownloadUrl = restOfUrl + '/' + encodeURIComponent(downloadName + '.' + extension); + const newWindow = window.open('about:blank'); + newWindow!.location.href = normalizedDownloadUrl; + return newWindow; + } + /** * Downloads the merged PDF file. * @@ -124,12 +141,4 @@ export class FileService { replaceAttachmentPrefixAndUnderscores(link: string): string { return link.replace(/AttachmentUnit_\d{4}-\d{2}-\d{2}T\d{2}-\d{2}-\d{2}-\d{3}_/, '').replace(/_/g, ' '); } - - /** - * Removes the prefix from the file name, and replaces underscore with spaces - * @param link - */ - replaceLectureAttachmentPrefixAndUnderscores(link: string): string { - return link.replace(/LectureAttachment_\d{4}-\d{2}-\d{2}T\d{2}-\d{2}-\d{2}-\d{3}_/, '').replace(/_/g, ' '); - } } From d3558023d936d5e1145645343a2c39111ab4def6 Mon Sep 17 00:00:00 2001 From: entholzer Date: Fri, 29 Nov 2024 08:07:47 +0100 Subject: [PATCH 2/4] fix build error --- .../attachment-unit/attachment-unit.component.ts | 2 +- .../component/lecture/lecture-attachments.component.spec.ts | 2 +- .../course-lecture-details.component.spec.ts | 6 +++--- .../spec/helpers/mocks/service/mock-file.service.ts | 3 +++ 4 files changed, 8 insertions(+), 5 deletions(-) diff --git a/src/main/webapp/app/overview/course-lectures/attachment-unit/attachment-unit.component.ts b/src/main/webapp/app/overview/course-lectures/attachment-unit/attachment-unit.component.ts index 4d70111dc9c9..ecc6c9f39f85 100644 --- a/src/main/webapp/app/overview/course-lectures/attachment-unit/attachment-unit.component.ts +++ b/src/main/webapp/app/overview/course-lectures/attachment-unit/attachment-unit.component.ts @@ -51,7 +51,7 @@ export class AttachmentUnitComponent extends LectureUnitDirective { fixture.detectChanges(); comp.isDownloadingAttachmentLink = undefined; expect(comp.isDownloadingAttachmentLink).toBeUndefined(); - comp.downloadAttachment('https://my/own/download/url'); + comp.downloadAttachment('https://my/own/download/url', 'test'); expect(comp.isDownloadingAttachmentLink).toBeUndefined(); })); diff --git a/src/test/javascript/spec/component/overview/course-lectures/course-lecture-details.component.spec.ts b/src/test/javascript/spec/component/overview/course-lectures/course-lecture-details.component.spec.ts index 101871555716..fce4233eb246 100644 --- a/src/test/javascript/spec/component/overview/course-lectures/course-lecture-details.component.spec.ts +++ b/src/test/javascript/spec/component/overview/course-lectures/course-lecture-details.component.spec.ts @@ -279,13 +279,13 @@ describe('CourseLectureDetailsComponent', () => { it('should download file for attachment', fakeAsync(() => { const fileService = TestBed.inject(FileService); - const downloadFileSpy = jest.spyOn(fileService, 'downloadFile'); + const downloadFileSpy = jest.spyOn(fileService, 'downloadFileByAttachmentName'); const attachment = getAttachmentUnit(lecture, 1, dayjs()).attachment!; - courseLecturesDetailsComponent.downloadAttachment(attachment.link); + courseLecturesDetailsComponent.downloadAttachment(attachment.link, attachment.name); expect(downloadFileSpy).toHaveBeenCalledOnce(); - expect(downloadFileSpy).toHaveBeenCalledWith(attachment.link); + expect(downloadFileSpy).toHaveBeenCalledWith(attachment.link, attachment.name); expect(courseLecturesDetailsComponent.isDownloadingLink).toBeUndefined(); })); diff --git a/src/test/javascript/spec/helpers/mocks/service/mock-file.service.ts b/src/test/javascript/spec/helpers/mocks/service/mock-file.service.ts index ae528e16f0ea..0e13c1e21090 100644 --- a/src/test/javascript/spec/helpers/mocks/service/mock-file.service.ts +++ b/src/test/javascript/spec/helpers/mocks/service/mock-file.service.ts @@ -8,6 +8,9 @@ export class MockFileService { downloadFile = () => { return { subscribe: (fn: (value: any) => void) => fn({ body: new Window() }) }; }; + downloadFileByAttachmentName = () => { + return { subscribe: (fn: (value: any) => void) => fn({ body: new Window() }) }; + }; getTemplateFile = () => { return of(); From 7be7e0ffd76aaa241b5892c336c6df46cc364811 Mon Sep 17 00:00:00 2001 From: entholzer Date: Fri, 29 Nov 2024 08:19:20 +0100 Subject: [PATCH 3/4] fix typo --- .../course-lectures/course-lecture-details.component.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/main/webapp/app/overview/course-lectures/course-lecture-details.component.ts b/src/main/webapp/app/overview/course-lectures/course-lecture-details.component.ts index 45a2525a5554..14cc75d10ef7 100644 --- a/src/main/webapp/app/overview/course-lectures/course-lecture-details.component.ts +++ b/src/main/webapp/app/overview/course-lectures/course-lecture-details.component.ts @@ -128,10 +128,10 @@ export class CourseLectureDetailsComponent extends AbstractScienceComponent impl return attachment.link.split('.').pop()!; } - downloadAttachment(downloadUrl?: string, donwloadName?: string): void { - if (!this.isDownloadingLink && downloadUrl && donwloadName) { + downloadAttachment(downloadUrl?: string, downloadName?: string): void { + if (!this.isDownloadingLink && downloadUrl && downloadName) { this.isDownloadingLink = downloadUrl; - this.fileService.downloadFileByAttachmentName(downloadUrl, donwloadName); + this.fileService.downloadFileByAttachmentName(downloadUrl, downloadName); this.isDownloadingLink = undefined; } } From c3f3601d84f58399cc6b84c411ab2c7fb903875e Mon Sep 17 00:00:00 2001 From: entholzer Date: Sun, 1 Dec 2024 10:17:25 +0100 Subject: [PATCH 4/4] fix tests and improve coverage --- .../AttachmentResourceIntegrationTest.java | 8 +- .../attachment-unit.component.spec.ts | 4 +- .../shared/http/file.service.spec.ts | 154 ++++++++++++++++++ 3 files changed, 162 insertions(+), 4 deletions(-) diff --git a/src/test/java/de/tum/cit/aet/artemis/lecture/AttachmentResourceIntegrationTest.java b/src/test/java/de/tum/cit/aet/artemis/lecture/AttachmentResourceIntegrationTest.java index 001b2ea8cda3..1bae5e4927f6 100644 --- a/src/test/java/de/tum/cit/aet/artemis/lecture/AttachmentResourceIntegrationTest.java +++ b/src/test/java/de/tum/cit/aet/artemis/lecture/AttachmentResourceIntegrationTest.java @@ -1,5 +1,6 @@ package de.tum.cit.aet.artemis.lecture; +import static org.apache.velocity.shaded.commons.io.FilenameUtils.getExtension; import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.Mockito.verify; import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; @@ -69,8 +70,11 @@ void initTestCase() { void createAttachment() throws Exception { Attachment actualAttachment = request.postWithMultipartFile("/api/attachments", attachment, "attachment", new MockMultipartFile("file", "test.txt", MediaType.TEXT_PLAIN_VALUE, "testContent".getBytes()), Attachment.class, HttpStatus.CREATED); - assertThat(actualAttachment.getLink()).isNotNull(); - MvcResult file = request.performMvcRequest(get(actualAttachment.getLink())).andExpect(status().isOk()).andExpect(content().contentType(MediaType.TEXT_PLAIN_VALUE)) + String actualLink = actualAttachment.getLink(); + assertThat(actualLink).isNotNull(); + // getLectureAttachment uses the provided file name to fetch the attachment which has that attachment name (not filename) + String linkWithCorrectFileName = actualLink.substring(0, actualLink.lastIndexOf('/') + 1) + attachment.getName() + "." + getExtension(actualAttachment.getLink()); + MvcResult file = request.performMvcRequest(get(linkWithCorrectFileName)).andExpect(status().isOk()).andExpect(content().contentType(MediaType.TEXT_PLAIN_VALUE)) .andReturn(); assertThat(file.getResponse().getContentAsByteArray()).isNotEmpty(); var expectedAttachment = attachmentRepository.findById(actualAttachment.getId()).orElseThrow(); diff --git a/src/test/javascript/spec/component/lecture-unit/attachment-unit/attachment-unit.component.spec.ts b/src/test/javascript/spec/component/lecture-unit/attachment-unit/attachment-unit.component.spec.ts index a65980b3725a..287b08031ac1 100644 --- a/src/test/javascript/spec/component/lecture-unit/attachment-unit/attachment-unit.component.spec.ts +++ b/src/test/javascript/spec/component/lecture-unit/attachment-unit/attachment-unit.component.spec.ts @@ -82,7 +82,7 @@ describe('AttachmentUnitComponent', () => { }); it('should handle download', () => { - const downloadFileSpy = jest.spyOn(fileService, 'downloadFile'); + const downloadFileSpy = jest.spyOn(fileService, 'downloadFileByAttachmentName'); const onCompletionEmitSpy = jest.spyOn(component.onCompletion, 'emit'); fixture.detectChanges(); @@ -113,7 +113,7 @@ describe('AttachmentUnitComponent', () => { }); it('should download attachment when clicked', () => { - const downloadFileSpy = jest.spyOn(fileService, 'downloadFile'); + const downloadFileSpy = jest.spyOn(fileService, 'downloadFileByAttachmentName'); fixture.detectChanges(); diff --git a/src/test/javascript/spec/component/shared/http/file.service.spec.ts b/src/test/javascript/spec/component/shared/http/file.service.spec.ts index 68a2b56a7faf..843fd61a12d3 100644 --- a/src/test/javascript/spec/component/shared/http/file.service.spec.ts +++ b/src/test/javascript/spec/component/shared/http/file.service.spec.ts @@ -3,6 +3,7 @@ import { HttpTestingController, provideHttpClientTesting } from '@angular/common import { TestBed } from '@angular/core/testing'; import { v4 as uuid } from 'uuid'; import { provideHttpClient } from '@angular/common/http'; +import { ProgrammingLanguage, ProjectType } from 'app/entities/programming/programming-exercise.model'; jest.mock('uuid', () => ({ v4: jest.fn(), @@ -77,4 +78,157 @@ describe('FileService', () => { expect(v4Mock).toHaveBeenCalledTimes(3); }); }); + + describe('getTemplateFile', () => { + it('should fetch the template file without project type', () => { + const language = ProgrammingLanguage.JAVA; + const expectedUrl = `api/files/templates/JAVA`; + const response = 'template content'; + + fileService.getTemplateFile(language).subscribe((data) => { + expect(data).toEqual(response); + }); + + const req = httpMock.expectOne({ + url: expectedUrl, + method: 'GET', + }); + expect(req.request.responseType).toBe('text'); + req.flush(response); + }); + + it('should fetch the template file with project type', () => { + const language = ProgrammingLanguage.JAVA; + const projectType = ProjectType.PLAIN_MAVEN; + const expectedUrl = `api/files/templates/JAVA/PLAIN_MAVEN`; + const response = 'template content'; + + fileService.getTemplateFile(language, projectType).subscribe((data) => { + expect(data).toEqual(response); + }); + + const req = httpMock.expectOne({ + url: expectedUrl, + method: 'GET', + }); + expect(req.request.responseType).toBe('text'); + req.flush(response); + }); + }); + + describe('downloadMergedFile', () => { + it('should download the merged PDF file', () => { + const lectureId = 123; + const expectedUrl = `api/files/attachments/lecture/${lectureId}/merge-pdf`; + const blobResponse = new Blob(['PDF content'], { type: 'application/pdf' }); + + fileService.downloadMergedFile(lectureId).subscribe((response) => { + expect(response.body).toEqual(blobResponse); + expect(response.status).toBe(200); + }); + + const req = httpMock.expectOne({ + url: expectedUrl, + method: 'GET', + }); + expect(req.request.responseType).toBe('blob'); + req.flush(blobResponse, { status: 200, statusText: 'OK' }); + }); + }); + + describe('getAeolusTemplateFile', () => { + it('should fetch the aeolus template file with all parameters', () => { + const language = ProgrammingLanguage.PYTHON; + const projectType = ProjectType.PLAIN; + const staticAnalysis = true; + const sequentialRuns = false; + const coverage = true; + const expectedUrl = `api/files/aeolus/templates/PYTHON/PLAIN?staticAnalysis=true&sequentialRuns=false&testCoverage=true`; + const response = 'aeolus template content'; + + fileService.getAeolusTemplateFile(language, projectType, staticAnalysis, sequentialRuns, coverage).subscribe((data) => { + expect(data).toEqual(response); + }); + + const req = httpMock.expectOne({ + url: expectedUrl, + method: 'GET', + }); + expect(req.request.responseType).toBe('text'); + req.flush(response); + }); + + it('should fetch the aeolus template file with missing optional parameters', () => { + const expectedUrl = `api/files/aeolus/templates/PYTHON?staticAnalysis=false&sequentialRuns=false&testCoverage=false`; + const response = 'aeolus template content'; + + fileService.getAeolusTemplateFile(ProgrammingLanguage.PYTHON).subscribe((data) => { + expect(data).toEqual(response); + }); + + const req = httpMock.expectOne({ + url: expectedUrl, + method: 'GET', + }); + expect(req.request.responseType).toBe('text'); + req.flush(response); + }); + }); + + describe('getTemplateCodeOfConduct', () => { + it('should fetch the template code of conduct', () => { + const expectedUrl = `api/files/templates/code-of-conduct`; + const response = 'code of conduct content'; + + fileService.getTemplateCodeOfCondcut().subscribe((data) => { + expect(data.body).toEqual(response); + }); + + const req = httpMock.expectOne({ + url: expectedUrl, + method: 'GET', + }); + expect(req.request.responseType).toBe('text'); + req.flush(response); + }); + }); + + describe('downloadFile', () => { + it('should open a new window with the normalized URL', () => { + const downloadUrl = 'http://example.com/files/some file name.txt'; + const encodedUrl = 'http://example.com/files/some%20file%20name.txt'; + const newWindowMock = { location: { href: '' } } as Window; + + jest.spyOn(window, 'open').mockReturnValue(newWindowMock); + + const newWindow = fileService.downloadFile(downloadUrl); + expect(newWindow).not.toBeNull(); + expect(newWindow!.location.href).toBe(encodedUrl); + }); + }); + + describe('downloadFileByAttachmentName', () => { + it('should open a new window with the normalized URL and attachment name', () => { + const downloadUrl = 'http://example.com/files/attachment.txt'; + const downloadName = 'newAttachment'; + const encodedUrl = 'http://example.com/files/newAttachment.txt'; + const newWindowMock = { location: { href: '' } } as Window; + + jest.spyOn(window, 'open').mockReturnValue(newWindowMock); + + const newWindow = fileService.downloadFileByAttachmentName(downloadUrl, downloadName); + expect(newWindow).not.toBeNull(); + expect(newWindow!.location.href).toBe(encodedUrl); + }); + }); + + describe('replaceAttachmentPrefixAndUnderscores', () => { + it('should replace the prefix and underscores in a file name', () => { + const fileName = 'AttachmentUnit_2023-01-01T00-00-00-000_some_file_name'; + const expected = 'some file name'; + + const result = fileService.replaceAttachmentPrefixAndUnderscores(fileName); + expect(result).toBe(expected); + }); + }); });