Communication: Fix broken link when referencing a lecture attachment in a post
#10164
Conversation
PaRangger
added
client
github-actions
bot
added
tests
and removed
communication
PaRangger
temporarily deployed
to
artemis-test4.artemis.cit.tum.de
GitHub Actions
Inactive
WalkthroughThe pull request introduces a new method Changes
Sequence DiagramsequenceDiagram
participant Component as PostingMarkdownEditor
participant Action as LectureAttachmentReferenceAction
participant Service as FileService
Component->>Action: Create with FileService
Action->>Service: Request attachment file URL
Service-->>Action: Return normalized URL
Action->>Action: Process lecture attachments
Possibly related PRs
Suggested labels
Suggested reviewers
Finishing Touches
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: 1
🧹 Nitpick comments (2)
src/main/webapp/app/shared/http/file.service.ts (1)
83-89: Add input validation to prevent potential errors.The method should validate input parameters to prevent runtime errors.
Apply this diff to add input validation:
createAttachmentFileUrl(downloadUrl: string, downloadName: string, encodeName: boolean) { + if (!downloadUrl || !downloadName) { + throw new Error('Download URL and name are required'); + } const downloadUrlComponents = downloadUrl.split('/'); - // take the last element - const extension = downloadUrlComponents.pop()!.split('.').pop(); + const fileName = downloadUrlComponents.pop(); + if (!fileName) { + throw new Error('Invalid download URL format'); + } + const extension = fileName.split('.').pop(); + if (!extension) { + throw new Error('File extension not found in URL'); + } const restOfUrl = downloadUrlComponents.join('/'); const encodedDownloadName = encodeName ? encodeURIComponent(downloadName + '.' + extension) : downloadName + '.' + extension; return restOfUrl + '/' + encodedDownloadName; }src/main/webapp/app/shared/monaco-editor/model/actions/communication/lecture-attachment-reference.action.ts (1)
50-71: Add error handling for attachment processing.The attachment processing logic should handle potential errors and provide meaningful feedback.
Apply this diff to improve error handling:
.map((lecture) => { - let attachmentCopy = cloneDeep(lecture.attachments); + try { + let attachmentCopy = cloneDeep(lecture.attachments); - if (attachmentCopy) { - attachmentCopy = attachmentCopy.map((attachment) => { - if (attachment.link && attachment.name) { - attachment.link = this.fileService.createAttachmentFileUrl(attachment.link!, attachment.name!, false); - } + if (attachmentCopy) { + attachmentCopy = attachmentCopy.map((attachment) => { + try { + if (attachment.link && attachment.name) { + attachment.link = this.fileService.createAttachmentFileUrl(attachment.link!, attachment.name!, false); + } + } catch (error) { + console.error(`Failed to process attachment ${attachment.name}: ${error}`); + } + return attachment; + }); + } - return attachment; - }); - } else { + return { + id: lecture.id!, + title: lecture.title!, + attachmentUnits: lecture.lectureUnits?.filter((unit) => unit.type === LectureUnitType.ATTACHMENT), + attachments: attachmentCopy, + }; + } catch (error) { + console.error(`Failed to process lecture ${lecture.title}: ${error}`); attachmentCopy = lecture.attachments; - } - - return { - id: lecture.id!, - title: lecture.title!, - attachmentUnits: lecture.lectureUnits?.filter((unit) => unit.type === LectureUnitType.ATTACHMENT), - attachments: attachmentCopy, - }; + return { + id: lecture.id!, + title: lecture.title!, + attachmentUnits: lecture.lectureUnits?.filter((unit) => unit.type === LectureUnitType.ATTACHMENT), + attachments: attachmentCopy, + }; + } });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/main/webapp/app/shared/http/file.service.ts(1 hunks)src/main/webapp/app/shared/metis/posting-markdown-editor/posting-markdown-editor.component.ts(3 hunks)src/main/webapp/app/shared/monaco-editor/model/actions/communication/lecture-attachment-reference.action.ts(2 hunks)src/test/javascript/spec/component/shared/metis/postings-markdown-editor/postings-markdown-editor.component.spec.ts(5 hunks)src/test/javascript/spec/component/shared/monaco-editor/monaco-editor-communication-action.integration.spec.ts(4 hunks)src/test/javascript/spec/helpers/mocks/service/mock-file.service.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
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}}
src/main/webapp/app/shared/http/file.service.ts (1)
src/test/javascript/spec/component/shared/monaco-editor/monaco-editor-communication-action.integration.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/shared/metis/postings-markdown-editor/postings-markdown-editor.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/main/webapp/app/shared/metis/posting-markdown-editor/posting-markdown-editor.component.ts (1)
src/main/webapp/app/shared/monaco-editor/model/actions/communication/lecture-attachment-reference.action.ts (1)
📓 Learnings (1)
src/test/javascript/spec/component/shared/monaco-editor/monaco-editor-communication-action.integration.spec.ts (1)
Learnt from: pzdr7
PR: ls1intum/Artemis#9407
File: src/test/javascript/spec/component/shared/monaco-editor/monaco-editor-communication-action.integration.spec.ts:46-46
Timestamp: 2024-11-12T12:51:40.391Z
Learning: In integration tests, it's acceptable to import the actual `MonacoEditorComponent` instead of mocking it.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: server-tests-postgres
- GitHub Check: server-tests-mysql
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (13)
src/main/webapp/app/shared/http/file.service.ts (1)
70-74: LGTM! Method refactored to use the new URL creation logic.The refactoring improves code reusability by utilizing the new
createAttachmentFileUrlmethod.src/main/webapp/app/shared/metis/posting-markdown-editor/posting-markdown-editor.component.ts (2)
68-68: LGTM! Proper dependency injection and usage.The FileService is correctly injected and properly passed to LectureAttachmentReferenceAction.
Also applies to: 124-124
Line range hint
1-1: Verify attachment link updates across the codebase.Let's ensure all attachment references are updated consistently.
Run the following script to check for any remaining unhandled attachment references:
✅ Verification successful
Attachment references are consistently maintained across the codebase
The verification shows that attachment references follow consistent patterns and are properly handled in both backend services and frontend components. All references use standardized URL structures and markdown syntax.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential unhandled attachment references # Search for attachment link patterns echo "Searching for attachment link patterns..." rg -A 2 "attachments/.*\.(pdf|png|jpg|jpeg|gif)" # Search for potential attachment URL construction echo "Searching for potential manual URL construction..." rg -A 2 "downloadUrl.*split.*attachments" # Search for potential attachment references in templates echo "Searching for attachment references in templates..." rg -A 2 '\[attachment\].*\(.*\)\[/attachment\]'Length of output: 12162
src/test/javascript/spec/component/shared/monaco-editor/monaco-editor-communication-action.integration.spec.ts (5)
33-34: LGTM!The imports are correctly placed and follow the established pattern.
40-40: LGTM!The
fileServicevariable declaration follows the pattern of other service declarations.
52-64: LGTM!The test module configuration correctly includes the
FileServiceprovider with its mock implementation.
71-72: LGTM!The
fileServiceinjection follows the pattern of other service injections.
301-304: LGTM!The attachment mapping correctly uses
fileService.createAttachmentFileUrlwhile preserving other attachment properties.src/test/javascript/spec/component/shared/metis/postings-markdown-editor/postings-markdown-editor.component.spec.ts (5)
39-42: LGTM!The imports are correctly placed and follow the established pattern. The
ListActionimport path has been corrected.
50-50: LGTM!The
fileServicevariable declaration follows the pattern of other service declarations.
126-126: LGTM!The test module configuration correctly includes the
FileServiceprovider with its mock implementation.
141-141: LGTM!The
fileServiceinjection follows the pattern of other service injections.
162-162: LGTM!The
LectureAttachmentReferenceActioninstantiation is consistently updated withfileServiceacross all test cases.Also applies to: 169-169, 194-194, 202-202
| } else { | ||
| attachmentCopy = lecture.attachments; | ||
| } |
There was a problem hiding this comment.
Isn't this step kind of redundant, as it's only reached when lecture.attachments is undefined, which means you are just overwriting the undefined with another undefined? Or am I missing something here
HawKhiem
temporarily deployed
to
artemis-test2.artemis.cit.tum.de
GitHub Actions
Inactive
ItsaaaMeMario
temporarily deployed
to
artemis-test4.artemis.cit.tum.de
GitHub Actions
Inactive
Checklist
General
Client
Motivation and Context
Currently, when referencing a lecture attachment in the markdown editor and creating a post, clicking the link results in a 404 error. However, the attachment itself is functional and accessible within the lecture.
Addresses #9943
Description
I changed the behaviour of generating the link so that it matches the behaviour inside the lecture. With this future posts will not run into the same issue when referencing lecture attachments.
If you rename the attachment after posting it will still break the link. We are currently working on a general update for uploaded files in #9970 and beyond.
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
Screenshots
Previous behaviour:
Summary by CodeRabbit
New Features
Refactor
Tests