From 01c71a2981893f9b7992306ea40741121dca5721 Mon Sep 17 00:00:00 2001 From: Stephan Krusche Date: Fri, 18 Apr 2025 16:20:13 +0200 Subject: [PATCH 01/10] Lectures: Prefer attachment units over lecture attachments --- docs/admin/accessRights.rst | 6 -- .../lecture/web/AttachmentResource.java | 25 ----- .../user-settings-container.component.spec.ts | 3 + .../lecture-attachments.component.html | 24 ++--- .../lecture-attachments.component.spec.ts | 95 ------------------- .../lecture-attachments.component.ts | 44 ++------- .../manage/lecture/lecture.component.spec.ts | 4 +- .../manage/lecture/lecture.component.ts | 2 +- .../services/attachment.service.spec.ts | 16 ---- .../manage/services/attachment.service.ts | 26 ----- .../lecture-attachment-reference.action.ts | 2 + src/main/webapp/i18n/de/global.json | 2 +- src/main/webapp/i18n/en/global.json | 2 +- .../AttachmentResourceIntegrationTest.java | 28 ------ 14 files changed, 23 insertions(+), 256 deletions(-) diff --git a/docs/admin/accessRights.rst b/docs/admin/accessRights.rst index e1b1c329c774..f7395c5c125e 100644 --- a/docs/admin/accessRights.rst +++ b/docs/admin/accessRights.rst @@ -200,12 +200,6 @@ Lectures +---------------------+------------+--------+--------------------+ | | Instructor | Editor | Teaching Assistant | +---------------------+------------+--------+--------------------+ -| Add attachment | ✔ | ✔ | | -+---------------------+------------+--------+--------------------+ -| Edit attachment | ✔ | ✔ | | -+---------------------+------------+--------+--------------------+ -| Delete attachment | ✔ | | | -+---------------------+------------+--------+--------------------+ | Add Lecture Unit | ✔ | ✔ | | +---------------------+------------+--------+--------------------+ | Edit Lecture Unit | ✔ | ✔ | | diff --git a/src/main/java/de/tum/cit/aet/artemis/lecture/web/AttachmentResource.java b/src/main/java/de/tum/cit/aet/artemis/lecture/web/AttachmentResource.java index e49eb12c7930..cfc5a1c5a2b0 100644 --- a/src/main/java/de/tum/cit/aet/artemis/lecture/web/AttachmentResource.java +++ b/src/main/java/de/tum/cit/aet/artemis/lecture/web/AttachmentResource.java @@ -4,7 +4,6 @@ import static de.tum.cit.aet.artemis.core.service.FilePathService.actualPathForPublicPath; import java.net.URI; -import java.net.URISyntaxException; import java.nio.file.Path; import java.util.List; import java.util.Optional; @@ -18,7 +17,6 @@ import org.springframework.web.bind.annotation.DeleteMapping; import org.springframework.web.bind.annotation.GetMapping; import org.springframework.web.bind.annotation.PathVariable; -import org.springframework.web.bind.annotation.PostMapping; import org.springframework.web.bind.annotation.PutMapping; import org.springframework.web.bind.annotation.RequestMapping; import org.springframework.web.bind.annotation.RequestParam; @@ -77,29 +75,6 @@ public AttachmentResource(AttachmentRepository attachmentRepository, GroupNotifi this.fileService = fileService; } - /** - * POST /attachments : Create a new attachment. - * - * @param attachment the attachment object to create - * @param file the file to save - * @return the ResponseEntity with status 201 (Created) and with body the new attachment, or with status 400 (Bad Request) if the attachment has already an ID - * @throws URISyntaxException if the Location URI syntax is incorrect - */ - @PostMapping(value = "attachments", consumes = MediaType.MULTIPART_FORM_DATA_VALUE) - @EnforceAtLeastEditor - public ResponseEntity createAttachment(@RequestPart Attachment attachment, @RequestPart MultipartFile file) throws URISyntaxException { - log.debug("REST request to save Attachment : {}", attachment); - attachment.setId(null); - - Path basePath = FilePathService.getLectureAttachmentFilePath().resolve(attachment.getLecture().getId().toString()); - Path savePath = fileService.saveFile(file, basePath, true); - attachment.setLink(FilePathService.publicPathForActualPathOrThrow(savePath, attachment.getLecture().getId()).toString()); - - Attachment result = attachmentRepository.save(attachment); - - return ResponseEntity.created(new URI("/api/lecture/attachments/" + result.getId())).body(result); - } - /** * PUT /attachments/:id : Updates an existing attachment. * diff --git a/src/main/webapp/app/core/user/settings/user-settings-container/user-settings-container.component.spec.ts b/src/main/webapp/app/core/user/settings/user-settings-container/user-settings-container.component.spec.ts index 1e946d80bcd7..e519c2fda59f 100644 --- a/src/main/webapp/app/core/user/settings/user-settings-container/user-settings-container.component.spec.ts +++ b/src/main/webapp/app/core/user/settings/user-settings-container/user-settings-container.component.spec.ts @@ -1,6 +1,8 @@ import { ComponentFixture, TestBed } from '@angular/core/testing'; import { ActivatedRoute, Router, RouterModule } from '@angular/router'; +import { ProfileService } from 'app/core/layouts/profiles/shared/profile.service'; import { MockNgbModalService } from 'test/helpers/mocks/service/mock-ngb-modal.service'; +import { MockProfileService } from 'test/helpers/mocks/service/mock-profile.service'; import { MockTranslateService } from 'test/helpers/mocks/service/mock-translate.service'; import { TranslateService } from '@ngx-translate/core'; import { NgbModal } from '@ng-bootstrap/ng-bootstrap'; @@ -28,6 +30,7 @@ describe('User Settings Container Component', () => { { provide: Router, useValue: router }, { provide: ActivatedRoute, useValue: new MockActivatedRoute() }, { provide: AccountService, useClass: MockAccountService }, + { provide: ProfileService, useClass: MockProfileService }, ], }).compileComponents(); fixture = TestBed.createComponent(UserSettingsContainerComponent); diff --git a/src/main/webapp/app/lecture/manage/lecture-attachments/lecture-attachments.component.html b/src/main/webapp/app/lecture/manage/lecture-attachments/lecture-attachments.component.html index 9072cb94ab75..bbbc64c333d7 100644 --- a/src/main/webapp/app/lecture/manage/lecture-attachments/lecture-attachments.component.html +++ b/src/main/webapp/app/lecture/manage/lecture-attachments/lecture-attachments.component.html @@ -78,17 +78,6 @@

- @if (viewButtonAvailable[attachment.id!]) { - - }
} @else { -
-
- -
-
+ + + } } diff --git a/src/main/webapp/app/lecture/manage/lecture-attachments/lecture-attachments.component.spec.ts b/src/main/webapp/app/lecture/manage/lecture-attachments/lecture-attachments.component.spec.ts index 5f20ba804012..7c1eda51a3b7 100644 --- a/src/main/webapp/app/lecture/manage/lecture-attachments/lecture-attachments.component.spec.ts +++ b/src/main/webapp/app/lecture/manage/lecture-attachments/lecture-attachments.component.spec.ts @@ -115,7 +115,6 @@ describe('LectureAttachmentsComponent', () => { attachmentService = TestBed.inject(AttachmentService); lectureService = TestBed.inject(LectureService); attachmentServiceFindAllByLectureIdStub = jest.spyOn(attachmentService, 'findAllByLectureId').mockReturnValue(of(new HttpResponse({ body: [...attachments] }))); - attachmentServiceCreateStub = jest.spyOn(attachmentService, 'create').mockReturnValue(of(new HttpResponse({ body: newAttachment }))); attachmentServiceUpdateStub = jest.spyOn(attachmentService, 'update').mockReturnValue(of(new HttpResponse({ body: newAttachment }))); }); }); @@ -143,92 +142,15 @@ describe('LectureAttachmentsComponent', () => { expect(findAllAttachmentsByLectureId).toHaveBeenCalledWith(42); })); - it('should accept file and add attachment to list', fakeAsync(() => { - fixture.detectChanges(); - jest.spyOn(attachmentService, 'create').mockReturnValue(of(new HttpResponse({ body: newAttachment }))); - const addAttachmentButton = fixture.debugElement.query(By.css('#add-attachment')); - expect(comp.attachmentToBeUpdatedOrCreated()).toBeUndefined(); - expect(addAttachmentButton).not.toBeNull(); - addAttachmentButton.nativeElement.click(); - fixture.detectChanges(); - comp.attachmentFile.set(new File([''], 'Test-File.pdf', { type: 'application/pdf' })); - const uploadAttachmentButton = fixture.debugElement.query(By.css('#upload-attachment')); - expect(uploadAttachmentButton).not.toBeNull(); - expect(comp.attachmentToBeUpdatedOrCreated()).not.toBeNull(); - comp.form.patchValue({ - attachmentName: 'Test File Name', - releaseDate: dayjs(), - }); - fixture.detectChanges(); - expect(uploadAttachmentButton.nativeElement.disabled).toBeFalse(); - uploadAttachmentButton.nativeElement.click(); - fixture.detectChanges(); - tick(); - expect(comp.attachments).toHaveLength(3); - expect(attachmentServiceFindAllByLectureIdStub).toHaveBeenCalledTimes(2); - })); - - it('should not accept too large file', fakeAsync(() => { - attachmentServiceCreateStub.mockReturnValue(throwError(() => new Error('File too large'))); - fixture.detectChanges(); - const addAttachmentButton = fixture.debugElement.query(By.css('#add-attachment')); - expect(comp.attachmentToBeUpdatedOrCreated()).toBeUndefined(); - expect(addAttachmentButton).not.toBeNull(); - addAttachmentButton.nativeElement.click(); - fixture.detectChanges(); - const fakeBlob = { name: 'Test-File.pdf', size: 100000000000000000 }; - comp.attachmentFile.set(fakeBlob as File); - const uploadAttachmentButton = fixture.debugElement.query(By.css('#upload-attachment')); - expect(uploadAttachmentButton).not.toBeNull(); - expect(comp.attachmentToBeUpdatedOrCreated()).not.toBeNull(); - comp.form.patchValue({ - attachmentName: 'Test File Name', - releaseDate: dayjs(), - }); - fixture.detectChanges(); - expect(uploadAttachmentButton.nativeElement.disabled).toBeFalse(); - uploadAttachmentButton.nativeElement.click(); - tick(); - fixture.detectChanges(); - const fileAlert = fixture.debugElement.query(By.css('#too-large-file-alert')); - expect(comp.attachments).toHaveLength(2); - expect(fileAlert).not.toBeNull(); - expect(attachmentServiceFindAllByLectureIdStub).toHaveBeenCalledOnce(); - })); - it('should exit saveAttachment', fakeAsync(() => { fixture.detectChanges(); comp.attachmentToBeUpdatedOrCreated.set(undefined); comp.saveAttachment(); expect(attachmentServiceFindAllByLectureIdStub).toHaveBeenCalledOnce(); - expect(attachmentServiceCreateStub).not.toHaveBeenCalled(); expect(attachmentServiceUpdateStub).not.toHaveBeenCalled(); expect(comp.attachmentToBeUpdatedOrCreated()).toBeUndefined(); })); - it('should reset on error for create', fakeAsync(() => { - const errorMessage = 'File too large'; - fixture.detectChanges(); - const attachment = { - lecture: comp.lecture, - attachmentType: AttachmentType.FILE, - version: 1, - uploadDate: dayjs(), - } as Attachment; - const file = new File([''], 'Test-File.pdf', { type: 'application/pdf' }); - comp.fileInput = { nativeElement: { value: 'Test-File.pdf' } }; - comp.attachmentToBeUpdatedOrCreated.set(attachment); - comp.attachmentFile.set(file); - const attachmentServiceCreateStub = jest.spyOn(attachmentService, 'create').mockReturnValue(throwError(() => new Error(errorMessage))); - comp.saveAttachment(); - expect(attachmentServiceCreateStub).toHaveBeenCalledExactlyOnceWith(attachment, file); - expect(comp.attachmentToBeUpdatedOrCreated()).toEqual(attachment); - expect(comp.attachmentFile()).toBeUndefined(); - expect(comp.erroredFile).toEqual(file); - expect(comp.errorMessage).toBe(errorMessage); - expect(comp.fileInput.nativeElement.value).toBe(''); - })); - it.each([true, false])( 'should reset on error for update: %s', fakeAsync((withFile: boolean) => { @@ -395,21 +317,4 @@ describe('LectureAttachmentsComponent', () => { expect(comp.attachmentToBeUpdatedOrCreated()?.link).toBe(myBlob1.name); expect(attachmentServiceFindAllByLectureIdStub).toHaveBeenCalledOnce(); })); - - describe('isViewButtonAvailable', () => { - it('should return true if the attachment link ends with .pdf', () => { - const attachment = { id: 1, link: 'example.pdf', attachmentType: 'FILE' } as Attachment; - expect(comp.isViewButtonAvailable(attachment.link!)).toBeTrue(); - }); - - it('should return false if the attachment link does not end with .pdf', () => { - const attachment = { id: 2, link: 'example.txt', attachmentType: 'FILE' } as Attachment; - expect(comp.isViewButtonAvailable(attachment.link!)).toBeFalse(); - }); - - it.each([['document.docx'], ['spreadsheet.xlsx'], ['presentation.pptx'], ['image.jpeg']])('should return false for common file extension %s', (link) => { - const attachment = { link }; - expect(comp.isViewButtonAvailable(attachment.link)).toBeFalse(); - }); - }); }); diff --git a/src/main/webapp/app/lecture/manage/lecture-attachments/lecture-attachments.component.ts b/src/main/webapp/app/lecture/manage/lecture-attachments/lecture-attachments.component.ts index ebc62e68993a..caf06badba17 100644 --- a/src/main/webapp/app/lecture/manage/lecture-attachments/lecture-attachments.component.ts +++ b/src/main/webapp/app/lecture/manage/lecture-attachments/lecture-attachments.component.ts @@ -4,22 +4,22 @@ import { HttpErrorResponse, HttpResponse } from '@angular/common/http'; import { Lecture } from 'app/lecture/shared/entities/lecture.model'; import dayjs from 'dayjs/esm'; import { Subject, Subscription } from 'rxjs'; -import { Attachment, AttachmentType } from 'app/lecture/shared/entities/attachment.model'; +import { Attachment } from 'app/lecture/shared/entities/attachment.model'; import { AttachmentService } from 'app/lecture/manage/services/attachment.service'; -import { faEye, faPaperclip, faPencilAlt, faQuestionCircle, faSpinner, faTimes, faTrash } from '@fortawesome/free-solid-svg-icons'; +import { faPaperclip, faPencilAlt, faQuestionCircle, faSpinner, faTimes, faTrash } from '@fortawesome/free-solid-svg-icons'; import { ACCEPTED_FILE_EXTENSIONS_FILE_BROWSER, ALLOWED_FILE_EXTENSIONS_HUMAN_READABLE } from 'app/shared/constants/file-extensions.constants'; import { LectureService } from 'app/lecture/manage/services/lecture.service'; import { FormBuilder, FormGroup, FormsModule, ReactiveFormsModule, Validators } from '@angular/forms'; import { toSignal } from '@angular/core/rxjs-interop'; import { FormDateTimePickerComponent } from 'app/shared/date-time-picker/date-time-picker.component'; -import { TranslateDirective } from '../../../shared/language/translate.directive'; +import { TranslateDirective } from 'app/shared/language/translate.directive'; import { NgClass } from '@angular/common'; import { FaIconComponent } from '@fortawesome/angular-fontawesome'; import { NgbTooltip } from '@ng-bootstrap/ng-bootstrap'; -import { DeleteButtonDirective } from '../../../shared/delete-dialog/directive/delete-button.directive'; +import { DeleteButtonDirective } from 'app/shared/delete-dialog/directive/delete-button.directive'; import { ArtemisDatePipe } from 'app/shared/pipes/artemis-date.pipe'; -import { ArtemisTranslatePipe } from '../../../shared/pipes/artemis-translate.pipe'; -import { HtmlForMarkdownPipe } from '../../../shared/pipes/html-for-markdown.pipe'; +import { ArtemisTranslatePipe } from 'app/shared/pipes/artemis-translate.pipe'; +import { HtmlForMarkdownPipe } from 'app/shared/pipes/html-for-markdown.pipe'; import { FileService } from 'app/shared/service/file.service'; export interface LectureAttachmentFormData { @@ -37,10 +37,10 @@ export interface LectureAttachmentFormData { TranslateDirective, NgClass, FaIconComponent, - RouterLink, NgbTooltip, DeleteButtonDirective, FormsModule, + RouterLink, ReactiveFormsModule, FormDateTimePickerComponent, ArtemisDatePipe, @@ -55,7 +55,6 @@ export class LectureAttachmentsComponent implements OnDestroy { protected readonly faPencilAlt = faPencilAlt; protected readonly faPaperclip = faPaperclip; protected readonly faQuestionCircle = faQuestionCircle; - protected readonly faEye = faEye; protected readonly allowedFileExtensions = ALLOWED_FILE_EXTENSIONS_HUMAN_READABLE; protected readonly acceptedFileExtensionsFileBrowser = ACCEPTED_FILE_EXTENSIONS_FILE_BROWSER; @@ -81,7 +80,6 @@ export class LectureAttachmentsComponent implements OnDestroy { notificationText?: string; erroredFile?: File; errorMessage?: string; - viewButtonAvailable: Record = {}; private dialogErrorSource = new Subject(); dialogError$ = this.dialogErrorSource.asObservable(); @@ -125,9 +123,6 @@ export class LectureAttachmentsComponent implements OnDestroy { loadAttachments(): void { this.attachmentService.findAllByLectureId(this.lecture().id!).subscribe((attachmentsResponse: HttpResponse) => { this.attachments = attachmentsResponse.body!; - this.attachments.forEach((attachment) => { - this.viewButtonAvailable[attachment.id!] = this.isViewButtonAvailable(attachment.link!); - }); }); } @@ -136,19 +131,6 @@ export class LectureAttachmentsComponent implements OnDestroy { this.routeDataSubscription?.unsubscribe(); } - isViewButtonAvailable(attachmentLink: string): boolean { - return attachmentLink.endsWith('.pdf') ?? false; - } - - addAttachment(): void { - const newAttachment = new Attachment(); - newAttachment.lecture = this.lecture(); - newAttachment.attachmentType = AttachmentType.FILE; - newAttachment.version = 0; - newAttachment.uploadDate = dayjs(); - this.attachmentToBeUpdatedOrCreated.set(newAttachment); - } - /** * If there is an attachment to save, it will be created or updated depending on its current state. The file will be automatically provided with the request. */ @@ -181,18 +163,6 @@ export class LectureAttachmentsComponent implements OnDestroy { }, error: (error: HttpErrorResponse) => this.handleFailedUpload(error), }); - } else { - this.attachmentService.create(this.attachmentToBeUpdatedOrCreated()!, this.attachmentFile()!).subscribe({ - next: (attachmentRes: HttpResponse) => { - this.attachments.push(attachmentRes.body!); - this.lectureService.findWithDetails(this.lecture().id!).subscribe((lectureResponse: HttpResponse) => { - this.lecture.set(lectureResponse.body!); - }); - this.loadAttachments(); - this.resetAttachmentFormVariables(); - }, - error: (error: HttpErrorResponse) => this.handleFailedUpload(error), - }); } } diff --git a/src/main/webapp/app/lecture/manage/lecture/lecture.component.spec.ts b/src/main/webapp/app/lecture/manage/lecture/lecture.component.spec.ts index fd0e2fb7442f..fd53589b8fce 100644 --- a/src/main/webapp/app/lecture/manage/lecture/lecture.component.spec.ts +++ b/src/main/webapp/app/lecture/manage/lecture/lecture.component.spec.ts @@ -129,7 +129,7 @@ describe('Lecture', () => { }, }, MockProvider(LectureService, { - findAllByCourseIdWithSlides: () => { + findAllByCourseId: () => { return of( new HttpResponse({ body: [pastLecture, pastLecture2, currentLecture, currentLecture2, currentLecture3, futureLecture, futureLecture2, unspecifiedLecture], @@ -169,7 +169,7 @@ describe('Lecture', () => { }); it('should fetch lectures when initialized', () => { - const findAllSpy = jest.spyOn(lectureService, 'findAllByCourseIdWithSlides'); + const findAllSpy = jest.spyOn(lectureService, 'findAllByCourseId'); lectureComponentFixture.detectChanges(); diff --git a/src/main/webapp/app/lecture/manage/lecture/lecture.component.ts b/src/main/webapp/app/lecture/manage/lecture/lecture.component.ts index 1e10a2313b18..fba49210d38a 100644 --- a/src/main/webapp/app/lecture/manage/lecture/lecture.component.ts +++ b/src/main/webapp/app/lecture/manage/lecture/lecture.component.ts @@ -181,7 +181,7 @@ export class LectureComponent implements OnInit, OnDestroy { private loadAll() { this.lectureService - .findAllByCourseIdWithSlides(this.courseId) + .findAllByCourseId(this.courseId) .pipe( filter((res: HttpResponse) => res.ok), map((res: HttpResponse) => res.body), diff --git a/src/main/webapp/app/lecture/manage/services/attachment.service.spec.ts b/src/main/webapp/app/lecture/manage/services/attachment.service.spec.ts index f47eb588dd17..29c259dd2281 100644 --- a/src/main/webapp/app/lecture/manage/services/attachment.service.spec.ts +++ b/src/main/webapp/app/lecture/manage/services/attachment.service.spec.ts @@ -48,22 +48,6 @@ describe('Attachment Service', () => { }); describe('Service methods', () => { - it('should create an attachment in the database', async () => { - const file = new File([], 'testName.txt'); - const returnedFromService = { ...elemDefault }; - const expected = { ...returnedFromService }; - service - .create(elemDefault, file) - .pipe(take(1)) - .subscribe((resp) => (expectedResult = resp)); - const req = httpMock.expectOne({ - url: resourceUrl, - method: 'POST', - }); - req.flush(returnedFromService); - expect(expectedResult.body).toEqual(expected); - }); - it.each([new File([], 'testName.txt'), undefined])('should create an attachment in the database with file %s', async (file: File | undefined) => { const returnedFromService = { ...elemDefault }; const expected = { ...returnedFromService }; diff --git a/src/main/webapp/app/lecture/manage/services/attachment.service.ts b/src/main/webapp/app/lecture/manage/services/attachment.service.ts index b82bceff586a..c9c0168b0926 100644 --- a/src/main/webapp/app/lecture/manage/services/attachment.service.ts +++ b/src/main/webapp/app/lecture/manage/services/attachment.service.ts @@ -18,32 +18,6 @@ export class AttachmentService { public resourceUrl = 'api/lecture/attachments'; - /** - * Create a new attachment - * @param attachment the attachment object to create - * @param file the file to save as an attachment - */ - create(attachment: Attachment, file: File): Observable { - const copy = this.convertAttachmentDatesFromClient(attachment); - // avoid potential issues when sending the attachment to the server - if (copy.attachmentUnit) { - copy.attachmentUnit.lecture = undefined; - copy.attachmentUnit.competencyLinks = undefined; - } - if (copy.lecture) { - copy.lecture.lectureUnits = undefined; - copy.lecture.course = undefined; - copy.lecture.posts = undefined; - } - - /** Ngsw-worker is bypassed temporarily to fix Chromium file upload issue - * See: https://issues.chromium.org/issues/374550348 - **/ - return this.http - .post(this.resourceUrl, this.createFormData(copy, file), { headers: { 'ngsw-bypass': 'true' }, observe: 'response' }) - .pipe(map((res: EntityResponseType) => this.convertAttachmentResponseDatesFromServer(res))); - } - /** * Update an existing attachment * @param attachmentId the id of the attachment to update diff --git a/src/main/webapp/app/shared/monaco-editor/model/actions/communication/lecture-attachment-reference.action.ts b/src/main/webapp/app/shared/monaco-editor/model/actions/communication/lecture-attachment-reference.action.ts index a0942e2b9185..76d1639e22d5 100644 --- a/src/main/webapp/app/shared/monaco-editor/model/actions/communication/lecture-attachment-reference.action.ts +++ b/src/main/webapp/app/shared/monaco-editor/model/actions/communication/lecture-attachment-reference.action.ts @@ -39,11 +39,13 @@ export class LectureAttachmentReferenceAction extends TextEditorAction { lecturesWithDetails: LectureWithDetails[] = []; constructor( + // TODO: use inject instead of constructor injection private readonly metisService: MetisService, private readonly lectureService: LectureService, private readonly fileService: FileService, ) { super(LectureAttachmentReferenceAction.ID, 'artemisApp.metis.editor.lecture'); + // TODO: do we really need all lectures and slides, this call is quite heavy and slow??? firstValueFrom(this.lectureService.findAllByCourseIdWithSlides(this.metisService.getCourse().id!)).then((response) => { const lectures = response.body; if (lectures) { diff --git a/src/main/webapp/i18n/de/global.json b/src/main/webapp/i18n/de/global.json index 8bdc7a6aa2a1..841cf496f8f1 100644 --- a/src/main/webapp/i18n/de/global.json +++ b/src/main/webapp/i18n/de/global.json @@ -245,7 +245,7 @@ "lecture": "Vorlesung", "attachments": "Anhänge", "saveAttachment": "Anhang speichern", - "addAttachment": "Anhang hinzufügen", + "addAttachment": "Anhang als Vorlesungseinheit hinzufügen", "assessmentDashboard": "Bewertung", "instructorDashboard": "Dashboard für Lehrende", "exerciseDashboard": "Aufgaben-Dashboard", diff --git a/src/main/webapp/i18n/en/global.json b/src/main/webapp/i18n/en/global.json index de0af4950dbc..b509b56202a7 100644 --- a/src/main/webapp/i18n/en/global.json +++ b/src/main/webapp/i18n/en/global.json @@ -245,7 +245,7 @@ "lecture": "Lectures", "attachments": "Attachments", "saveAttachment": "Save attachment", - "addAttachment": "Add attachment", + "addAttachment": "Add attachment as lecture unit", "assessmentDashboard": "Assessment", "instructorDashboard": "Instructor Dashboard", "exerciseDashboard": "Exercise Dashboard", 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 89783b662913..2e8d14d3d987 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,12 +1,7 @@ package de.tum.cit.aet.artemis.lecture; -import static de.tum.cit.aet.artemis.core.config.Constants.ARTEMIS_FILE_PATH_PREFIX; -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; -import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.content; -import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -17,7 +12,6 @@ import org.springframework.http.MediaType; import org.springframework.mock.web.MockMultipartFile; import org.springframework.security.test.context.support.WithMockUser; -import org.springframework.test.web.servlet.MvcResult; import org.springframework.util.LinkedMultiValueMap; import de.tum.cit.aet.artemis.core.domain.Course; @@ -66,28 +60,6 @@ void initTestCase() { attachment.setLecture(lecture); } - @Test - @WithMockUser(username = TEST_PREFIX + "instructor1", roles = "INSTRUCTOR") - void createAttachment() throws Exception { - Attachment actualAttachment = request.postWithMultipartFile("/api/lecture/attachments", attachment, "attachment", - new MockMultipartFile("file", "test.txt", MediaType.TEXT_PLAIN_VALUE, "testContent".getBytes()), Attachment.class, HttpStatus.CREATED); - 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()); - String requestUrl = String.format("%s%s", ARTEMIS_FILE_PATH_PREFIX, linkWithCorrectFileName); - MvcResult file = request.performMvcRequest(get(requestUrl)).andExpect(status().isOk()).andExpect(content().contentType(MediaType.TEXT_PLAIN_VALUE)).andReturn(); - assertThat(file.getResponse().getContentAsByteArray()).isNotEmpty(); - var expectedAttachment = attachmentRepository.findById(actualAttachment.getId()).orElseThrow(); - assertThat(actualAttachment).isEqualTo(expectedAttachment); - } - - @Test - @WithMockUser(username = TEST_PREFIX + "instructor1", roles = "INSTRUCTOR") - void createAttachment_noFile() throws Exception { - request.postWithMultipartFile("/api/lecture/attachments", attachment, "attachment", null, Attachment.class, HttpStatus.BAD_REQUEST); - } - @ParameterizedTest @WithMockUser(username = TEST_PREFIX + "instructor1", roles = "INSTRUCTOR") @ValueSource(booleans = { true, false }) From 0e1ec961fb94db669bd51d290131f9e8070444ff Mon Sep 17 00:00:00 2001 From: Stephan Krusche Date: Fri, 18 Apr 2025 16:46:25 +0200 Subject: [PATCH 02/10] remove unused code --- jest.config.js | 2 +- .../lecture/repository/LectureRepository.java | 16 ------- .../artemis/lecture/web/LectureResource.java | 46 +++---------------- .../manage/services/lecture.service.spec.ts | 16 ------- .../manage/services/lecture.service.ts | 17 ------- .../lecture-attachment-reference.action.ts | 12 ++--- .../lecture/LectureIntegrationTest.java | 7 --- 7 files changed, 13 insertions(+), 103 deletions(-) diff --git a/jest.config.js b/jest.config.js index 56dd43349d51..cf7034b9d962 100644 --- a/jest.config.js +++ b/jest.config.js @@ -92,7 +92,7 @@ module.exports = { global: { // TODO: in the future, the following values should increase to at least 90% statements: 88.98, - branches: 75.18, + branches: 75.15, functions: 82.98, lines: 89.04, }, diff --git a/src/main/java/de/tum/cit/aet/artemis/lecture/repository/LectureRepository.java b/src/main/java/de/tum/cit/aet/artemis/lecture/repository/LectureRepository.java index 427086613fb7..52d96473943a 100644 --- a/src/main/java/de/tum/cit/aet/artemis/lecture/repository/LectureRepository.java +++ b/src/main/java/de/tum/cit/aet/artemis/lecture/repository/LectureRepository.java @@ -109,17 +109,6 @@ public interface LectureRepository extends ArtemisJpaRepository { """) Optional findByIdWithLectureUnitsAndAttachments(@Param("lectureId") Long lectureId); - @Query(""" - SELECT lecture - FROM Lecture lecture - LEFT JOIN FETCH lecture.lectureUnits lectureUnit - LEFT JOIN FETCH lectureUnit.attachment luAttachment - LEFT JOIN FETCH lectureUnit.slides slides - LEFT JOIN FETCH lecture.attachments - WHERE lecture.id = :lectureId - """) - Optional findByIdWithLectureUnitsAndSlidesAndAttachments(@Param("lectureId") long lectureId); - @Query(""" SELECT lecture FROM Lecture lecture @@ -200,11 +189,6 @@ default Lecture findByIdWithLectureUnitsAndAttachmentsElseThrow(Long lectureId) return getValueElseThrow(findByIdWithLectureUnitsAndAttachments(lectureId), lectureId); } - @NotNull - default Lecture findByIdWithLectureUnitsAndSlidesAndAttachmentsElseThrow(long lectureId) { - return getValueElseThrow(findByIdWithLectureUnitsAndSlidesAndAttachments(lectureId), lectureId); - } - @Query(""" SELECT new de.tum.cit.aet.artemis.core.dto.CourseContentCount( COUNT(l.id), diff --git a/src/main/java/de/tum/cit/aet/artemis/lecture/web/LectureResource.java b/src/main/java/de/tum/cit/aet/artemis/lecture/web/LectureResource.java index fd72f9f9c36b..507a0a39dbe8 100644 --- a/src/main/java/de/tum/cit/aet/artemis/lecture/web/LectureResource.java +++ b/src/main/java/de/tum/cit/aet/artemis/lecture/web/LectureResource.java @@ -28,7 +28,6 @@ import org.springframework.web.bind.annotation.RestController; import de.tum.cit.aet.artemis.atlas.api.CompetencyApi; -import de.tum.cit.aet.artemis.atlas.config.AtlasNotPresentException; import de.tum.cit.aet.artemis.communication.domain.conversation.Channel; import de.tum.cit.aet.artemis.communication.repository.conversation.ChannelRepository; import de.tum.cit.aet.artemis.communication.service.conversation.ChannelService; @@ -320,31 +319,6 @@ public ResponseEntity getLectureWithDetails(@PathVariable Long lectureI return ResponseEntity.ok(lecture); } - /** - * GET /lectures/:lectureId/details-with-slides : get the "lectureId" lecture with active lecture units and with slides. - * - * @param lectureId the lectureId of the lecture to retrieve - * @return the ResponseEntity with status 200 (OK) and with body the lecture including posts, lecture units and competencies, or with status 404 (Not Found) - */ - @GetMapping("lectures/{lectureId}/details-with-slides") - @EnforceAtLeastStudent - public ResponseEntity getLectureWithDetailsAndSlides(@PathVariable long lectureId) { - log.debug("REST request to get lecture {} with details with slides ", lectureId); - Lecture lecture = lectureRepository.findByIdWithLectureUnitsAndSlidesAndAttachmentsElseThrow(lectureId); - Course course = lecture.getCourse(); - if (course == null) { - return ResponseEntity.badRequest().build(); - } - User user = userRepository.getUserWithGroupsAndAuthorities(); - authCheckService.checkIsAllowedToSeeLectureElseThrow(lecture, user); - - competencyApi.orElseThrow(() -> new AtlasNotPresentException(CompetencyApi.class)).addCompetencyLinksToExerciseUnits(lecture); - lectureService.filterActiveAttachmentUnits(lecture); - lectureService.filterActiveAttachments(lecture, user); - lectureService.filterHiddenPagesOfAttachmentUnits(lecture); - return ResponseEntity.ok(lecture); - } - /** * GET /lectures/:lectureId/title : Returns the title of the lecture with the given id * @@ -370,20 +344,12 @@ private Lecture filterLectureContentForUser(Lecture lecture, User user) { Set exercisesWithAllInformationNeeded = exerciseService .loadExercisesWithInformationForDashboard(exercisesUserIsAllowedToSee.stream().map(Exercise::getId).collect(Collectors.toSet()), user); - List lectureUnitsUserIsAllowedToSee = lecture.getLectureUnits().stream().filter(lectureUnit -> { - if (lectureUnit == null) { - return false; - } - if (lectureUnit instanceof ExerciseUnit) { - return ((ExerciseUnit) lectureUnit).getExercise() != null && authCheckService.isAllowedToSeeLectureUnit(lectureUnit, user) - && exercisesWithAllInformationNeeded.contains(((ExerciseUnit) lectureUnit).getExercise()); - } - else if (lectureUnit instanceof AttachmentUnit) { - return ((AttachmentUnit) lectureUnit).getAttachment() != null && authCheckService.isAllowedToSeeLectureUnit(lectureUnit, user); - } - else { - return authCheckService.isAllowedToSeeLectureUnit(lectureUnit, user); - } + List lectureUnitsUserIsAllowedToSee = lecture.getLectureUnits().stream().filter(lectureUnit -> switch (lectureUnit) { + case null -> false; + case ExerciseUnit exerciseUnit -> exerciseUnit.getExercise() != null && authCheckService.isAllowedToSeeLectureUnit(lectureUnit, user) + && exercisesWithAllInformationNeeded.contains(exerciseUnit.getExercise()); + case AttachmentUnit attachmentUnit -> attachmentUnit.getAttachment() != null && authCheckService.isAllowedToSeeLectureUnit(lectureUnit, user); + default -> authCheckService.isAllowedToSeeLectureUnit(lectureUnit, user); }).peek(lectureUnit -> { lectureUnit.setCompleted(lectureUnit.isCompletedFor(user)); diff --git a/src/main/webapp/app/lecture/manage/services/lecture.service.spec.ts b/src/main/webapp/app/lecture/manage/services/lecture.service.spec.ts index cd052ab2555a..004517de0d84 100644 --- a/src/main/webapp/app/lecture/manage/services/lecture.service.spec.ts +++ b/src/main/webapp/app/lecture/manage/services/lecture.service.spec.ts @@ -95,22 +95,6 @@ describe('Lecture Service', () => { expect(expectedResult.body).toEqual(expected); }); - it('should find a lecture with details and with slides in the database', async () => { - const returnedFromService = { ...elemDefault }; - const expected = { ...returnedFromService, posts: [] }; - const lectureId = elemDefault.id!; - service - .findWithDetailsWithSlides(lectureId) - .pipe(take(1)) - .subscribe((resp) => (expectedResult = resp)); - const req = httpMock.expectOne({ - url: `${resourceUrl}/${lectureId}/details-with-slides`, - method: 'GET', - }); - req.flush(returnedFromService); - expect(expectedResult.body).toEqual(expected); - }); - it('should find a lecture in the database', async () => { const returnedFromService = { ...elemDefault }; const expected = { ...returnedFromService }; diff --git a/src/main/webapp/app/lecture/manage/services/lecture.service.ts b/src/main/webapp/app/lecture/manage/services/lecture.service.ts index 689c172e5014..fa9b09fa2df3 100644 --- a/src/main/webapp/app/lecture/manage/services/lecture.service.ts +++ b/src/main/webapp/app/lecture/manage/services/lecture.service.ts @@ -60,23 +60,6 @@ export class LectureService { ); } - findWithDetailsWithSlides(lectureId: number): Observable { - return this.http.get(`${this.resourceUrl}/${lectureId}/details-with-slides`, { observe: 'response' }).pipe( - map((res: EntityResponseType) => { - if (res.body) { - // insert an empty list to avoid additional calls in case the list is empty on the server (because then it would be undefined in the client) - if (res.body.posts === undefined) { - res.body.posts = []; - } - } - this.convertLectureResponseDatesFromServer(res); - this.setAccessRightsLecture(res.body); - this.sendTitlesToEntityTitleService(res?.body); - return res; - }), - ); - } - query(req?: any): Observable { const options = createRequestOption(req); return this.http.get(this.resourceUrl, { params: options, observe: 'response' }).pipe( diff --git a/src/main/webapp/app/shared/monaco-editor/model/actions/communication/lecture-attachment-reference.action.ts b/src/main/webapp/app/shared/monaco-editor/model/actions/communication/lecture-attachment-reference.action.ts index 76d1639e22d5..241567dc385b 100644 --- a/src/main/webapp/app/shared/monaco-editor/model/actions/communication/lecture-attachment-reference.action.ts +++ b/src/main/webapp/app/shared/monaco-editor/model/actions/communication/lecture-attachment-reference.action.ts @@ -1,4 +1,5 @@ import { TextEditorAction } from 'app/shared/monaco-editor/model/actions/text-editor-action.model'; +import { inject } from '@angular/core'; import { MetisService } from 'app/communication/service/metis.service'; import { firstValueFrom } from 'rxjs'; import { LectureService } from 'app/lecture/manage/services/lecture.service'; @@ -38,12 +39,11 @@ export class LectureAttachmentReferenceAction extends TextEditorAction { lecturesWithDetails: LectureWithDetails[] = []; - constructor( - // TODO: use inject instead of constructor injection - private readonly metisService: MetisService, - private readonly lectureService: LectureService, - private readonly fileService: FileService, - ) { + private readonly metisService = inject(MetisService); + private readonly lectureService = inject(LectureService); + private readonly fileService = inject(FileService); + + constructor() { super(LectureAttachmentReferenceAction.ID, 'artemisApp.metis.editor.lecture'); // TODO: do we really need all lectures and slides, this call is quite heavy and slow??? firstValueFrom(this.lectureService.findAllByCourseIdWithSlides(this.metisService.getCourse().id!)).then((response) => { diff --git a/src/test/java/de/tum/cit/aet/artemis/lecture/LectureIntegrationTest.java b/src/test/java/de/tum/cit/aet/artemis/lecture/LectureIntegrationTest.java index a07654d287c6..7a042f38d299 100644 --- a/src/test/java/de/tum/cit/aet/artemis/lecture/LectureIntegrationTest.java +++ b/src/test/java/de/tum/cit/aet/artemis/lecture/LectureIntegrationTest.java @@ -267,13 +267,6 @@ void getLectureForCourse_WithLectureUnitsWithSlides_shouldGetLecturesWithLecture assertThat(filteredLecture.getLectureUnits()).contains(attachmentUnitWithSlides); AttachmentUnit attachmentUnit = (AttachmentUnit) filteredLecture.getLectureUnits().getFirst(); assertThat(attachmentUnit.getSlides()).hasSize(numberOfSlides); - - Lecture lectureWithDetails = request.get("/api/lecture/lectures/" + lectureWithSlides.getId() + "/details-with-slides", HttpStatus.OK, Lecture.class); - - assertThat(lectureWithDetails.getLectureUnits()).hasSize(1); // we only have one lecture unit which is attachmentUnitWithSlides - assertThat(lectureWithDetails.getLectureUnits()).contains(attachmentUnitWithSlides); - AttachmentUnit attachmentUnitDetails = (AttachmentUnit) lectureWithDetails.getLectureUnits().getFirst(); - assertThat(attachmentUnitDetails.getSlides()).hasSize(numberOfSlides); } @Test From 5ad241c69d3ec423df57de7589c4d255e6c57c79 Mon Sep 17 00:00:00 2001 From: Stephan Krusche Date: Fri, 18 Apr 2025 20:20:41 +0200 Subject: [PATCH 03/10] general code improvements --- docs/dev/guidelines/client-design.rst | 2 +- .../posting-reactions-bar.component.html | 2 +- .../course-sidebar/course-sidebar.component.html | 2 +- .../app/lecture/manage/lecture/lecture.component.ts | 12 ++++++------ .../lecture-unit/lecture-unit.component.ts | 3 ++- .../lecture-attachment-reference.action.ts | 11 +++++------ .../shared/pipe/meeting-pattern.pipe.ts | 4 +++- 7 files changed, 19 insertions(+), 17 deletions(-) diff --git a/docs/dev/guidelines/client-design.rst b/docs/dev/guidelines/client-design.rst index fd0aafc31b59..3d1670c5882f 100644 --- a/docs/dev/guidelines/client-design.rst +++ b/docs/dev/guidelines/client-design.rst @@ -209,7 +209,7 @@ For example, you could add a reactive flag to your component that indicates whet isDark = false; themeSubscription: Subscription; - constructor(private themeService: ThemeService) {} + private themeService = inject(ThemeService); ngOnInit() { this.themeSubscription = this.themeService.getCurrentThemeObservable().subscribe((theme) => { diff --git a/src/main/webapp/app/communication/posting-reactions-bar/posting-reactions-bar.component.html b/src/main/webapp/app/communication/posting-reactions-bar/posting-reactions-bar.component.html index a6a10097ef41..086a3eefc023 100644 --- a/src/main/webapp/app/communication/posting-reactions-bar/posting-reactions-bar.component.html +++ b/src/main/webapp/app/communication/posting-reactions-bar/posting-reactions-bar.component.html @@ -49,7 +49,7 @@ } - @for (reactionMetaData of reactionMetaDataMap | keyvalue; track reactionMetaData) { + @for (reactionMetaData of reactionMetaDataMap | keyvalue; track reactionMetaData.key) { @if (isEmojiCount()) {
- @if (isEditMode()) { -
-

-

- -

- -
- } @if (isEditMode()) {