Lectures: Fix file names for downloads with chromium browsers
#9899
Conversation
github-actions
bot
added
server
SimonEntholzer
added
lecture
github-actions
bot
removed
the
lecture
WalkthroughThe pull request introduces several modifications across multiple files, primarily focusing on enhancing the handling of attachment names and download processes. Key changes include updating method signatures to accept both attachment names and URLs, modifying the logic for file downloads, and introducing a new method in the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 pmd (7.7.0)src/test/java/de/tum/cit/aet/artemis/lecture/AttachmentResourceIntegrationTest.javaThe following rules are missing or misspelled in your ruleset file category/vm/bestpractices.xml: BooleanInstantiation, DontImportJavaLang, DuplicateImports, EmptyFinallyBlock, EmptyIfStmt, EmptyInitializer, EmptyStatementBlock, EmptyStatementNotInLoop, EmptySwitchStatements, EmptySynchronizedBlock, EmptyTryBlock, EmptyWhileStmt, ExcessiveClassLength, ExcessiveMethodLength, ImportFromSamePackage, MissingBreakInSwitch, SimplifyBooleanAssertion. Please check your ruleset configuration. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (5)
src/main/webapp/app/overview/course-lectures/attachment-unit/attachment-unit.component.ts (1)
Line range hint
33-41: Update getFileName to align with new naming strategyThe
getFileNamemethod still uses URL-based naming while the PR objective is to use user-defined attachment names.Consider updating the method to use the attachment name first, falling back to URL-based naming:
getFileName(): string { - if (this.lectureUnit().attachment?.link) { - const link = this.lectureUnit().attachment!.link!; - const filename = link.substring(link.lastIndexOf('/') + 1); - return this.fileService.replaceAttachmentPrefixAndUnderscores(filename); + const attachment = this.lectureUnit().attachment; + if (attachment?.name) { + return attachment.name; + } else if (attachment?.link) { + const link = attachment.link; + const filename = link.substring(link.lastIndexOf('/') + 1); + return this.fileService.replaceAttachmentPrefixAndUnderscores(filename); } return ''; }🧰 Tools
🪛 GitHub Check: client-compilation
[failure] 54-54:
Property 'downloadFileWithName' does not exist on type 'FileService'.🪛 GitHub Check: client-tests-selected
[failure] 54-54:
Property 'downloadFileWithName' does not exist on type 'FileService'.🪛 GitHub Check: client-tests
[failure] 54-54:
Property 'downloadFileWithName' does not exist on type 'FileService'.src/main/webapp/app/shared/http/file.service.ts (2)
91-100: Consider refactoring to reduce code duplicationThe
downloadFileByAttachmentNamemethod shares similar logic withdownloadFile. Consider extracting common functionality.+private openDownloadWindow(url: string): Window | null { + const normalizedUrl = encodeURIComponent(url); + const newWindow = window.open('about:blank'); + if (!newWindow) { + window.location.href = normalizedUrl; + return null; + } + newWindow.location.href = normalizedUrl; + return newWindow; +} downloadFile(downloadUrl: string) { const downloadUrlComponents = downloadUrl.split('/'); const fileName = downloadUrlComponents.pop()!; const restOfUrl = downloadUrlComponents.join('/'); - const normalizedDownloadUrl = restOfUrl + '/' + encodeURIComponent(fileName); - const newWindow = window.open('about:blank'); - newWindow!.location.href = normalizedDownloadUrl; - return newWindow; + return this.openDownloadWindow(`${restOfUrl}/${fileName}`); } downloadFileByAttachmentName(downloadUrl: string, downloadName: string) { // ... extension extraction logic ... - const normalizedDownloadUrl = restOfUrl + '/' + encodeURIComponent(downloadName + '.' + extension); - const newWindow = window.open('about:blank'); - newWindow!.location.href = normalizedDownloadUrl; - return newWindow; + return this.openDownloadWindow(`${restOfUrl}/${downloadName}.${extension}`); }
85-90: Documentation needs improvementThe JSDoc comment should include:
- Return type description
- Possible errors that might be thrown
- Example usage
/** * 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 + * @returns {Window | null} The window object for the download, or null if popup was blocked + * @throws {Error} When download URL or name is invalid + * @example + * // Download file with custom name + * fileService.downloadFileByAttachmentName('http://example.com/file.pdf', 'custom-name'); */src/main/webapp/app/overview/course-lectures/course-lecture-details.component.ts (1)
131-136: Consider adding download progress indicatorTo improve user experience, consider showing a progress indicator during downloads.
downloadAttachment(downloadUrl: string, downloadName: string): void { if (!this.isDownloadingLink) { this.isDownloadingLink = downloadUrl; + // Show loading indicator in template when isDownloadingLink matches the current attachment + this.alertService.info('artemisApp.attachment.download.started'); this.fileService.downloadFileByAttachmentName(downloadUrl, downloadName).pipe( finalize(() => { this.isDownloadingLink = undefined; }) ).subscribe({ + next: () => { + this.alertService.success('artemisApp.attachment.download.success'); + }, error: (error: HttpErrorResponse) => { this.alertService.error('artemisApp.attachment.download.error'); } }); }src/main/java/de/tum/cit/aet/artemis/core/web/FileResource.java (1)
424-425: Consider improving error message clarityThe error message uses
attachmentNamedirectly, which includes the extension. Consider usinggetBaseName(attachmentName)in the error message for consistency with the filtering logic.- .orElseThrow(() -> new EntityNotFoundException("Attachment", attachmentName)); + .orElseThrow(() -> new EntityNotFoundException("Attachment", getBaseName(attachmentName)));
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
src/main/java/de/tum/cit/aet/artemis/core/web/FileResource.java(4 hunks)src/main/webapp/app/lecture/lecture-attachments.component.html(1 hunks)src/main/webapp/app/lecture/lecture-attachments.component.ts(1 hunks)src/main/webapp/app/overview/course-lectures/attachment-unit/attachment-unit.component.ts(1 hunks)src/main/webapp/app/overview/course-lectures/course-lecture-details.component.html(1 hunks)src/main/webapp/app/overview/course-lectures/course-lecture-details.component.ts(1 hunks)src/main/webapp/app/shared/http/file.service.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
src/main/java/de/tum/cit/aet/artemis/core/web/FileResource.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/webapp/app/lecture/lecture-attachments.component.html (1)
Pattern src/main/webapp/**/*.html: @if and @for are new and valid Angular syntax replacing *ngIf and *ngFor. They should always be used over the old style.
src/main/webapp/app/lecture/lecture-attachments.component.ts (1)
src/main/webapp/app/overview/course-lectures/attachment-unit/attachment-unit.component.ts (1)
src/main/webapp/app/overview/course-lectures/course-lecture-details.component.html (1)
Pattern src/main/webapp/**/*.html: @if and @for are new and valid Angular syntax replacing *ngIf and *ngFor. They should always be used over the old style.
src/main/webapp/app/overview/course-lectures/course-lecture-details.component.ts (1)
src/main/webapp/app/shared/http/file.service.ts (1)
🪛 GitHub Check: client-compilation
src/main/webapp/app/overview/course-lectures/attachment-unit/attachment-unit.component.ts
[failure] 54-54:
Property 'downloadFileWithName' does not exist on type 'FileService'.
🪛 GitHub Check: client-tests-selected
src/main/webapp/app/overview/course-lectures/attachment-unit/attachment-unit.component.ts
[failure] 54-54:
Property 'downloadFileWithName' does not exist on type 'FileService'.
🪛 GitHub Check: client-tests
src/main/webapp/app/overview/course-lectures/attachment-unit/attachment-unit.component.ts
[failure] 54-54:
Property 'downloadFileWithName' does not exist on type 'FileService'.
🔇 Additional comments (9)
src/main/webapp/app/shared/http/file.service.ts (1)
91-100: Verify browser compatibility
Since this PR specifically addresses Chromium browser issues, we should verify the implementation across different browsers.
src/main/webapp/app/overview/course-lectures/course-lecture-details.component.ts (1)
131-136: Verify the impact on template bindings
The method signature change requires updates to the template bindings.
✅ Verification successful
Template binding is correctly aligned with method signature
The template binding in course-lecture-details.component.html correctly provides both required parameters:
attachment.linkmaps todownloadUrlattachment.namemaps todonwloadName
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check template usage of downloadAttachment method
# Expected: All calls should provide both URL and name parameters
# Search for downloadAttachment usage in templates
rg -t html 'downloadAttachment\(' src/main/webapp/app/overview/course-lectures/
Length of output: 300
src/main/webapp/app/overview/course-lectures/course-lecture-details.component.html (1)
Line range hint 38-44: Verify merged PDF download functionality.
Since this PR fixes file naming issues, ensure that the merged PDF download also uses proper file names.
Let's check the implementation:
✅ Verification successful
Let me gather more information about the file service implementation to verify the merged PDF download functionality.
Merged PDF download functionality is properly implemented
The implementation in course-lecture-details.component.ts shows that the merged PDF download uses the lecture title as the filename:
downloadStream(blob.body, 'application/pdf', this.lecture?.title ?? 'Lecture');This is consistent with the file naming improvements in this PR, as it:
- Uses a meaningful name (lecture title) for the downloaded file
- Has a fallback to 'Lecture' if title is not available
- Includes the correct file extension (.pdf)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the merged PDF download implementation
# Check the downloadMergedFiles method implementation
ast-grep --pattern 'downloadMergedFiles() {
$$$
}'
# Look for any file service usage in the merged files download
rg -A 5 'downloadMergedFiles|FileService.*download.*PDF'
Length of output: 2604
Script:
#!/bin/bash
# Check the file service implementation for merged file download
ast-grep --pattern 'downloadMergedFile($_) {
$$$
}'
# Look for any blob handling or file naming in the file service
rg -A 5 'downloadMergedFile|createBlobUrl|getFilename'
Length of output: 10958
src/main/webapp/app/lecture/lecture-attachments.component.ts (1)
203-208: Implementation aligns with PR objectives.
The changes correctly implement the file name handling requirements for Chromium browsers by passing both the attachment name and URL to the download service.
Add JSDoc documentation for better maintainability.
Consider adding JSDoc documentation to describe the parameters and purpose of the method.
+ /**
+ * Downloads an attachment using both the attachment name and URL.
+ * @param downloadName The name to be used for the downloaded file
+ * @param downloadUrl The URL from which to download the file
+ */
downloadAttachment(downloadName: string, downloadUrl: string): void {Verify template updates.
Ensure that all template files calling this method have been updated to provide both parameters.
✅ Verification successful
Template update is correctly implemented
The template file lecture-attachments.component.html correctly calls downloadAttachment with both required parameters: attachment.name and attachment.link, with null coalescing operators for safety.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all template files that might call downloadAttachment
# and verify they provide both parameters
# Search for downloadAttachment calls in template files
rg -t html 'downloadAttachment\s*\(' src/main/webapp/app/lecture/
Length of output: 286
src/main/webapp/app/lecture/lecture-attachments.component.html (1)
64-64: LGTM! The change properly addresses the file naming issues.
The updated click handler now correctly passes both the attachment name and link, which will help fix the file naming issues in Chromium browsers. The null coalescing operator is properly used for both parameters to prevent undefined/null errors.
Let's verify that the component's TypeScript file has been updated to match this new method signature:
✅ Verification successful
The component's TypeScript implementation correctly matches the template usage
The TypeScript implementation of downloadAttachment already accepts both downloadName and downloadUrl parameters, perfectly matching the template's usage. The method signature and implementation are properly aligned with the changes made to fix the file naming issues in Chromium browsers.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the downloadAttachment method signature in the component file
# Expected: Method should accept both name and link parameters
ast-grep --pattern 'downloadAttachment(name: string, link: string)' src/main/webapp/app/lecture/lecture-attachments.component.ts
Length of output: 130
Script:
#!/bin/bash
# Let's try a broader pattern to find the downloadAttachment method implementation
ast-grep --pattern 'downloadAttachment($_) {
$$$
}'
Length of output: 55
Script:
#!/bin/bash
# As a backup, let's also search using ripgrep
rg "downloadAttachment" -A 5 src/main/webapp/app/lecture/lecture-attachments.component.ts
Length of output: 426
Script:
#!/bin/bash
# Let's also check if the file exists
fd "lecture-attachments.component.ts" src/
Length of output: 103
src/main/java/de/tum/cit/aet/artemis/core/web/FileResource.java (4)
4-5: LGTM: Appropriate use of FilenameUtils
Using Apache Commons IO's FilenameUtils is a good choice for handling file names and extensions in a platform-independent way.
414-415: LGTM: Improved parameter naming and documentation
The parameter rename from filename to attachmentName better reflects its purpose and usage.
Also applies to: 418-421
490-490: LGTM: Proper file extension handling
The change correctly combines the attachment name with its extension, ensuring proper file downloads across different browsers.
Line range hint 414-434: Verify similar download endpoints
Let's check if other file download endpoints in the codebase might need similar fixes for consistent behavior across browsers.
Also applies to: 490-490
✅ Verification successful
Let me gather more specific information about file download endpoints.
Based on the comprehensive search results, I can now provide the final response:
All file download endpoints follow consistent patterns and security checks
The codebase shows consistent implementation across file download endpoints:
- All endpoints use
buildFileResponsewith proper path handling - Each endpoint implements appropriate authorization checks
- File name handling is consistent across different file types (attachments, submissions, etc.)
- The reviewed endpoint follows the same pattern as other similar endpoints in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find similar patterns in other endpoints that might need the same fix
# Look for ResponseEntity<byte[]> endpoints that might handle file downloads
rg -l "ResponseEntity<byte\[\]>" --type java
# Look for specific file handling patterns that might need fixing
ast-grep --pattern 'ResponseEntity<byte[]> $method($_) {
$$$
return buildFileResponse($_, $_, $_);
}'
Length of output: 1481
Script:
#!/bin/bash
# Search for file download related endpoints in FileResource.java
rg -A 5 "@GetMapping.*files.*" src/main/java/de/tum/cit/aet/artemis/core/web/FileResource.java
# Search for buildFileResponse usage to find all file download endpoints
rg -B 5 "buildFileResponse" src/main/java/de/tum/cit/aet/artemis/core/web/FileResource.java
# Look for similar patterns in other resources
rg -A 5 "@GetMapping.*download.*" --type java
Length of output: 16564
src/main/webapp/app/overview/course-lectures/attachment-unit/attachment-unit.component.ts
Outdated
Show resolved
Hide resolved
src/main/webapp/app/overview/course-lectures/course-lecture-details.component.ts
Outdated
Show resolved
Hide resolved
src/main/webapp/app/overview/course-lectures/course-lecture-details.component.ts
Outdated
Show resolved
Hide resolved
src/main/webapp/app/overview/course-lectures/course-lecture-details.component.html
Show resolved
Hide resolved
|
github-actions
bot
added
the
deployment-error
HawKhiem
added
deploy:artemis-test3
and removed
deployment-error
|
github-actions
bot
added
the
deployment-error
HawKhiem
added
deploy:artemis-test3
and removed
deployment-error
|
github-actions
bot
added
the
deployment-error
HawKhiem
removed
the
deployment-error
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
src/test/javascript/spec/helpers/mocks/service/mock-file.service.ts (1)
Line range hint
1-24: Consider standardizing mock implementations across the service.The service uses two different approaches for mocking:
- RxJS
of()operator (e.g.,downloadMergedFile,getTemplateFile)- Manual subscription objects (e.g.,
downloadFile,downloadFileByAttachmentName)This inconsistency could make the mock service harder to maintain and understand.
Consider standardizing all methods to use RxJS operators for consistency:
import { Observable, of, throwError } from 'rxjs'; export class MockFileService { - downloadMergedFile = () => { - return of({ body: null }); - }; + downloadMergedFile = (): Observable<{ body: null }> => { + return of({ body: null }); + }; - downloadFile = () => { - return { subscribe: (fn: (value: any) => void) => fn({ body: new Window() }) }; - }; + downloadFile = (): Observable<{ body: Window }> => { + return of({ body: new Window() }); + }; - downloadFileByAttachmentName = () => { - return { subscribe: (fn: (value: any) => void) => fn({ body: new Window() }) }; - }; + downloadFileByAttachmentName = ( + url: string, + attachmentName: string + ): Observable<{ body: Window }> => { + return url && attachmentName + ? of({ body: new Window() }) + : throwError(() => new Error('Invalid URL or attachment name')); + }; getTemplateFile = () => { return of(); }; replaceLectureAttachmentPrefixAndUnderscores = (link: string) => link; replaceAttachmentPrefixAndUnderscores = (link: string) => link; }This approach:
- Provides consistent usage of RxJS throughout the service
- Improves type safety with explicit return types
- Makes error handling more idiomatic using
throwError- Aligns better with Angular's reactive programming patterns
src/test/javascript/spec/component/overview/course-lectures/course-lecture-details.component.spec.ts (1)
288-289: Consider adding test cases for error scenariosWhile the current expectation correctly verifies the happy path using the recommended
toHaveBeenCalledWithmatcher, consider adding test cases for:
- Error handling when the download fails
- State management verification (isDownloadingLink) during the download process
- Edge cases with missing or invalid attachment names
Example test case to add:
it('should handle download errors gracefully', fakeAsync(() => { const fileService = TestBed.inject(FileService); const downloadFileSpy = jest.spyOn(fileService, 'downloadFileByAttachmentName') .mockRejectedValue(new Error('Download failed')); const attachment = getAttachmentUnit(lecture, 1, dayjs()).attachment!; courseLecturesDetailsComponent.downloadAttachment(attachment.link, attachment.name); tick(); expect(downloadFileSpy).toHaveBeenCalledOnce(); expect(courseLecturesDetailsComponent.isDownloadingLink).toBeFalsy(); }));src/test/javascript/spec/component/lecture/lecture-attachments.component.spec.ts (1)
364-364: Enhance test coverage for downloadAttachment.While the test verifies the basic functionality, consider adding these test cases to ensure robust error handling and browser compatibility:
- Test error scenarios when download fails
- Verify filename handling across different browsers (especially Chromium vs Firefox)
- Test with various file types and extensions
- Verify the correct content-disposition header is set
Example test cases to add:
it('should handle download errors', fakeAsync(() => { fixture.detectChanges(); const fileService = TestBed.inject(FileService); jest.spyOn(fileService, 'downloadFileByAttachmentName').mockReturnValue(throwError(() => new Error('Download failed'))); comp.downloadAttachment('https://example.com/file', 'test.pdf'); tick(); expect(comp.isDownloadingAttachmentLink).toBeUndefined(); // Verify error handling })); it('should set correct filename for different browsers', fakeAsync(() => { fixture.detectChanges(); const fileService = TestBed.inject(FileService); const downloadSpy = jest.spyOn(fileService, 'downloadFileByAttachmentName'); comp.downloadAttachment('https://example.com/file', 'test.pdf'); tick(); expect(downloadSpy).toHaveBeenCalledWith('https://example.com/file', 'test.pdf'); }));
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
src/main/webapp/app/overview/course-lectures/attachment-unit/attachment-unit.component.ts(1 hunks)src/test/javascript/spec/component/lecture/lecture-attachments.component.spec.ts(1 hunks)src/test/javascript/spec/component/overview/course-lectures/course-lecture-details.component.spec.ts(1 hunks)src/test/javascript/spec/helpers/mocks/service/mock-file.service.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/webapp/app/overview/course-lectures/attachment-unit/attachment-unit.component.ts
🧰 Additional context used
📓 Path-based instructions (3)
src/test/javascript/spec/component/lecture/lecture-attachments.component.spec.ts (1)
Pattern src/test/javascript/spec/**/*.ts: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}
src/test/javascript/spec/component/overview/course-lectures/course-lecture-details.component.spec.ts (1)
Pattern src/test/javascript/spec/**/*.ts: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}
src/test/javascript/spec/helpers/mocks/service/mock-file.service.ts (1)
Pattern src/test/javascript/spec/**/*.ts: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}
🔇 Additional comments (2)
src/test/javascript/spec/component/overview/course-lectures/course-lecture-details.component.spec.ts (2)
282-282: LGTM: Spy correctly set up for the new download method
The spy is properly configured to track calls to the new downloadFileByAttachmentName method, which aligns with the PR's objective to improve file naming during downloads.
285-285: LGTM: Method call correctly updated with new signature
The test properly validates the new behavior by passing both the attachment link and name to the downloadAttachment method.
SimonEntholzer
dismissed stale reviews from sawys777 and kaancayli
via
c3f3601
github-actions
bot
added
the
lecture
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (6)
src/test/javascript/spec/component/lecture-unit/attachment-unit/attachment-unit.component.spec.ts (3)
Line range hint
85-92: Enhance test coverage for downloadFileByAttachmentName parametersWhile the test verifies the method call and completion emission, it should also verify that the correct attachment name and URL are passed to the new method.
Consider updating the test like this:
const downloadFileSpy = jest.spyOn(fileService, 'downloadFileByAttachmentName'); const onCompletionEmitSpy = jest.spyOn(component.onCompletion, 'emit'); fixture.detectChanges(); component.handleDownload(); expect(downloadFileSpy).toHaveBeenCalledOnce(); +expect(downloadFileSpy).toHaveBeenCalledExactlyOnceWith('test', '/path/to/file/test.pdf'); expect(onCompletionEmitSpy).toHaveBeenCalledOnce();
Line range hint
116-124: Verify parameters in click handler testSimilar to the previous test, this one should verify that clicking the button triggers the download with correct parameters.
Update the test to verify parameter passing:
const downloadFileSpy = jest.spyOn(fileService, 'downloadFileByAttachmentName'); fixture.detectChanges(); const viewIsolatedButton = fixture.debugElement.query(By.css('#view-isolated-button')); viewIsolatedButton.nativeElement.click(); fixture.detectChanges(); expect(downloadFileSpy).toHaveBeenCalledOnce(); +expect(downloadFileSpy).toHaveBeenCalledExactlyOnceWith('test', '/path/to/file/test.pdf');
Line range hint
94-114: Add test cases for different file extensionsWhile the current test suite covers icon selection for different file types, consider adding test cases that verify the download behavior with different file extensions to ensure the PR's objective of fixing file naming issues is thoroughly tested.
Consider adding a test like this:
it.each([ ['pdf', 'document.pdf'], ['doc', 'document.doc'], ['txt', 'document.txt'] ])('should download file with correct extension: %s', (extension: string, filename: string) => { const downloadFileSpy = jest.spyOn(fileService, 'downloadFileByAttachmentName'); component.lectureUnit().attachment!.name = filename; component.lectureUnit().attachment!.link = `/path/to/file/${filename}`; fixture.detectChanges(); component.handleDownload(); expect(downloadFileSpy).toHaveBeenCalledExactlyOnceWith(filename, `/path/to/file/${filename}`); });src/test/java/de/tum/cit/aet/artemis/lecture/AttachmentResourceIntegrationTest.java (1)
73-77: Improve test readability and coverage.While the changes correctly verify the file name fix:
- Consider extracting the file name construction logic to a helper method for better readability:
- String linkWithCorrectFileName = actualLink.substring(0, actualLink.lastIndexOf('/') + 1) + attachment.getName() + "." + getExtension(actualAttachment.getLink()); + String linkWithCorrectFileName = constructAttachmentLink(actualLink, attachment.getName());
- Add test cases using
@ParameterizedTestto verify handling of different file extensions:@ParameterizedTest @ValueSource(strings = {"test.pdf", "test.txt", "test.doc"}) void createAttachmentWithDifferentExtensions(String fileName) { // Test implementation }src/test/javascript/spec/component/shared/http/file.service.spec.ts (2)
82-117: Consider adding error case tests for getTemplateFile.While the happy path cases are well covered, consider adding tests for:
- Invalid programming language
- Invalid project type
- Server error responses
139-176: Add boundary tests for getAeolusTemplateFile.Consider adding tests for:
- Invalid combinations of parameters
- Edge cases where some parameters are undefined/null
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
src/test/java/de/tum/cit/aet/artemis/lecture/AttachmentResourceIntegrationTest.java(2 hunks)src/test/javascript/spec/component/lecture-unit/attachment-unit/attachment-unit.component.spec.ts(2 hunks)src/test/javascript/spec/component/shared/http/file.service.spec.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
src/test/javascript/spec/component/lecture-unit/attachment-unit/attachment-unit.component.spec.ts (1)
Pattern src/test/javascript/spec/**/*.ts: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}
src/test/java/de/tum/cit/aet/artemis/lecture/AttachmentResourceIntegrationTest.java (1)
Pattern src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true
src/test/javascript/spec/component/shared/http/file.service.spec.ts (1)
Pattern src/test/javascript/spec/**/*.ts: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}
src/test/java/de/tum/cit/aet/artemis/lecture/AttachmentResourceIntegrationTest.java
Show resolved
Hide resolved
SimonEntholzer
requested review from
florian-glombik,
sawys777 and
kevinfischer4
SimonEntholzer
requested review from
coolchock,
sachmii,
kaancayli and
HawKhiem
SimonEntholzer
temporarily deployed
to
artemis-test2.artemis.cit.tum.de
GitHub Actions
Inactive
Malekos74
temporarily deployed
to
artemis-test2.artemis.cit.tum.de
GitHub Actions
Inactive
Checklist
General
Server
Client
Motivation and Context
There are still issues with the attachment downloads for lectures and lecture units:
Cause: We send the file without extension to the client, firefox manages to automatically deduct the file type + extension from the content, and appends the extension, and chrome simply doesn't.
Description
Steps for Testing
Testserver States
Note
These badges show the state of the test servers.
Green = Currently available, Red = Currently locked
Click on the badges to get to the test servers.
Review Progress
Code Review
Manual Tests
Test Coverage
Client
Server
Screenshots
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes