Lectures: Fix lecture unit file attachment names
#9721
Conversation
github-actions
bot
added
the
client
SimonEntholzer
changed the title
Lectures: Fix attachment file names
SimonEntholzer
changed the title
Lectures: Fix attachment file namesLectures: Fix lecture unit file attachment names
WalkthroughThe changes in this pull request focus on enhancing file name handling and downloading functionality within the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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: 0
🧹 Outside diff range and nitpick comments (2)
src/main/webapp/app/overview/course-lectures/attachment-unit/attachment-unit.component.ts (2)
40-41: LGTM! Consider adding null check for better defensive programming.The filename extraction logic is correct and aligns with the PR objective of cleaning up file names.
Consider adding a null check before substring operation:
-const filename = link.substring(link.lastIndexOf('/') + 1); +const filename = link?.substring(link.lastIndexOf('/') + 1) ?? '';
59-65: Consider making the prefix pattern more maintainable and robust.While the regex implementation works correctly, there are a few improvements that could make it more maintainable:
Consider these enhancements:
-private 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, ' '); +private readonly ATTACHMENT_PREFIX_PATTERN = /AttachmentUnit_\d{4}-\d{2}-\d{2}T\d{2}-\d{2}-\d{2}-\d{3}_/; + +private replaceAttachmentPrefixAndUnderscores(link: string): string { + if (!link) return ''; + return link + .replace(this.ATTACHMENT_PREFIX_PATTERN, '') + .replace(/_/g, ' ') + .trim(); }Benefits:
- The regex pattern is now configurable and reusable
- Added null check for defensive programming
- Trim whitespace from the result
- Better readability with method chaining
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
src/main/webapp/app/overview/course-lectures/attachment-unit/attachment-unit.component.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/main/webapp/app/overview/course-lectures/attachment-unit/attachment-unit.component.ts (1)
🔇 Additional comments (1)
src/main/webapp/app/overview/course-lectures/attachment-unit/attachment-unit.component.ts (1)
54-54: Verify safe handling of modified file paths in FileService.
While the filename cleanup improves user experience, we need to ensure that the FileService correctly handles the modified paths and maintains the proper mapping to actual files.
Let's check the FileService implementation:
✅ Verification successful
Based on the FileService implementation shown in the output, I can now generate the final response:
File path handling in FileService is secure and appropriate
The FileService's downloadFile method (lines 74-83) implements proper URL handling:
- Splits the URL into components and handles the filename separately
- Uses
encodeURIComponent()on the filename part to safely encode special characters - Reconstructs the URL with proper encoding before initiating the download
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for FileService's downloadFile method implementation
ast-grep --pattern 'class FileService {
$$$
downloadFile($$$) {
$$$
}
$$$
}'
# Look for any path handling or validation in FileService
rg -A 5 'class FileService'
Length of output: 12037
ItsaaaMeMario
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/test/javascript/spec/component/lecture-unit/attachment-unit/attachment-unit.component.spec.ts (2)
98-104: LGTM! Icon mappings are comprehensive.The test cases cover a good range of file types with appropriate icon mappings.
Consider adding test cases to verify the new file name formatting functionality mentioned in the PR objectives:
it('should remove prefix from file name', () => { component.lectureUnit().attachment!.name = 'prefix_test_file.pdf'; fixture.detectChanges(); expect(component.getFileName()).toBe('test file.pdf'); });
Based on the findings, I can now generate the final response since I've found the implementation of the prefix removal functionality in the component that needs test coverage.
Add test coverage for file name formatting functionality
The test file is missing coverage for the
replaceAttachmentPrefixAndUnderscoresfunctionality which:
- Removes timestamp prefixes matching pattern
AttachmentUnit_YYYY-MM-DDThh-mm-ss-SSS_- Replaces underscores with spaces
Add test cases like:
'AttachmentUnit_2024-01-01T12-00-00-000_test_file.pdf'→'test file.pdf''test_file.pdf'→'test file.pdf'🔗 Analysis chain
Line range hint
1-143: Add test coverage for file name prefix removal.While the test file is well-structured and follows best practices, it's missing coverage for the main PR objective - removing prefixes from file attachment names. Consider adding test cases to verify:
- Prefix removal from displayed file names
- Prefix removal from downloaded file names
- Underscore replacement in file names
Let me gather more information about the file name formatting functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if there are any existing tests for file name formatting # Test: Search for test cases related to file name formatting rg -A 5 "getFileName|replaceAttachmentPrefixAndUnderscores" "src/test/javascript/spec"Length of output: 4215
Script:
#!/bin/bash # Search for the implementation of file name formatting in the component ast-grep --pattern 'getFileName() { $$$ }' # Search for any prefix removal or underscore replacement logic rg -A 5 "replacePrefix|replaceUnderscores|replace.*Prefix|replace.*Underscore" "src"Length of output: 3153
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
src/test/javascript/spec/component/lecture-unit/attachment-unit/attachment-unit.component.spec.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
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}}
🔇 Additional comments (1)
src/test/javascript/spec/component/lecture-unit/attachment-unit/attachment-unit.component.spec.ts (1)
12-25: LGTM! Icon imports are well-organized.
The FontAwesome icon imports follow best practices by using named imports and grouping related icons together.
HawKhiem
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
Ah I see, you're downloading it from the instructor's lecture attachment view. That's a different component, I'll fix it there too |
SimonEntholzer
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
GitHub Actions
Inactive
|
github-actions
bot
added
the
deployment-error
Checklist
General
Client
Motivation and Context
The file names in the lecture unit have long cryptic prefixes, which are not relevant for users:
When the user downloads the file, it's named like shown.
Description
This PR removes the prefix for the displayed name, and for the downloaded file.
Steps for Testing
Prerequisites:
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
Summary by CodeRabbit
New Features
Bug Fixes
These changes aim to provide a better user experience when handling and downloading attachment files.