Communication: Fix an issue when referencing lectures that contain brackets in title
#9528
Conversation
…on/fix-referencing-slides
github-actions
bot
added
tests
client
PaRangger
added
component:Communication
communication
PaRangger
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
GitHub Actions
Inactive
WalkthroughThe changes in this pull request involve modifications to 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: 1
🧹 Outside diff range and nitpick comments (9)
src/main/webapp/app/shared/monaco-editor/model/actions/communication/lecture-attachment-reference.action.ts (5)
112-115: LGTM with a minor suggestion for readability.The changes look good and address the PR objective of fixing issues with brackets in titles. The use of the
sanitizeStringForMarkdownEditormethod ensures that the lecture title is properly sanitized before being inserted into the editor.Consider extracting the string template into a separate constant for improved readability:
const lectureReferenceTemplate = `[lecture]${this.sanitizeStringForMarkdownEditor(lecture.title)}(${this.metisService.getLinkForLecture(lecture.id.toString())})[/lecture]`; this.replaceTextAtCurrentSelection(editor, lectureReferenceTemplate);This change would make the code more readable and easier to maintain.
120-120: LGTM with a minor suggestion for readability.The changes look good and address the PR objective of fixing issues with brackets in attachment names. The use of the
sanitizeStringForMarkdownEditormethod ensures that the attachment name is properly sanitized before being inserted into the editor.Consider extracting the string template into a separate constant for improved readability:
const attachmentReferenceTemplate = `[attachment]${this.sanitizeStringForMarkdownEditor(attachment.name)}(${shortLink})[/attachment]`; this.replaceTextAtCurrentSelection(editor, attachmentReferenceTemplate);This change would make the code more readable and easier to maintain.
127-130: LGTM with a minor suggestion for readability.The changes look good and address the PR objective of fixing issues with brackets in attachment unit names. The use of the
sanitizeStringForMarkdownEditormethod ensures that the attachment unit name is properly sanitized before being inserted into the editor.Consider extracting the string template into a separate constant for improved readability:
const slideReferenceTemplate = `[slide]${this.sanitizeStringForMarkdownEditor(attachmentUnit.name)} Slide ${slide.slideNumber}(${shortLinkWithoutFileName})[/slide]`; this.replaceTextAtCurrentSelection(editor, slideReferenceTemplate);This change would make the code more readable and easier to maintain.
135-136: LGTM with a minor suggestion for readability.The changes look good and address the PR objective of fixing issues with brackets in attachment unit names. The use of the
sanitizeStringForMarkdownEditormethod ensures that the attachment unit name is properly sanitized before being inserted into the editor.Consider extracting the string template into a separate constant for improved readability:
const attachmentUnitReferenceTemplate = `[lecture-unit]${this.sanitizeStringForMarkdownEditor(attachmentUnit.name)}(${shortLink})[/lecture-unit]`; this.replaceTextAtCurrentSelection(editor, attachmentUnitReferenceTemplate);This change would make the code more readable and easier to maintain.
138-154: LGTM with a minor optimization suggestion.The new
sanitizeStringForMarkdownEditormethod effectively addresses the PR objective by removing brackets from the input string. The implementation is concise, handles undefined input, and follows TypeScript best practices.Consider using a regular expression for a more efficient implementation:
private sanitizeStringForMarkdownEditor(str: string | undefined): string | undefined { if (str === undefined) { return str; } return str.replace(/[\(\)\[\]]/g, ''); }This change would achieve the same result with a single
replaceoperation, potentially improving performance for longer strings.src/test/javascript/spec/component/shared/monaco-editor/monaco-editor-communication-action.integration.spec.ts (4)
Line range hint
23-23: Avoid full module import of 'monaco-editor'Importing the entire
monaco-editormodule can increase bundle size and affect performance. Consider importing only the necessary members to optimize the build.Apply this diff to import only the required parts:
-import * as monaco from 'monaco-editor'; +import { languages, Position } from 'monaco-editor';Ensure that all references to
monacoin the code are updated accordingly.
298-299: Use robust methods for extracting the short linkUsing
split('attachments/')to extractshortLinkmay be fragile if the URL structure changes. Consider using URL parsing utilities or regular expressions to reliably extract the desired part of the link.For example:
const url = new URL(newAttachment.link!, 'http://localhost'); const shortLink = url.pathname.split('/attachments/')[1];This approach reduces the dependency on the exact format of the link.
312-315: Avoid mutating shared objects in testsDirectly modifying
lecture.titleand then restoring it can lead to unintended side effects if other tests rely on the original value. To prevent this, create a copy of thelectureobject with the modified title.Apply this change:
-const lecture = lectureAttachmentReferenceAction.lecturesWithDetails[0]; -const previousTitle = lecture.title; -lecture.title = lectureNameWithBrackets; +const lecture = { ...lectureAttachmentReferenceAction.lecturesWithDetails[0], title: lectureNameWithBrackets };This ensures that the original
lectureobject remains unmodified.
328-335: Avoid mutating shared objects in testsChanging
attachmentUnit.namedirectly may affect other tests. Consider cloning theattachmentUnitobject before modifying it to avoid side effects.Apply this change:
-const attachmentUnit = lecture.attachmentUnits![0]; -const previousName = attachmentUnit.name; -attachmentUnit.name = attachmentUnitNameWithBrackets; +const attachmentUnit = { ...lecture.attachmentUnits![0], name: attachmentUnitNameWithBrackets };This creates a new
attachmentUnitinstance for this test case.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- src/main/webapp/app/shared/monaco-editor/model/actions/communication/lecture-attachment-reference.action.ts (1 hunks)
- src/test/javascript/spec/component/shared/monaco-editor/monaco-editor-communication-action.integration.spec.ts (2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/main/webapp/app/shared/monaco-editor/model/actions/communication/lecture-attachment-reference.action.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}}
🔇 Additional comments (2)
src/main/webapp/app/shared/monaco-editor/model/actions/communication/lecture-attachment-reference.action.ts (1)
Line range hint
1-154: Overall implementation looks great!The changes in this file effectively address the PR objective of fixing issues with brackets in lecture titles, attachment names, and other references. The new
sanitizeStringForMarkdownEditormethod is well-implemented and used consistently throughout the class.Key points:
- The sanitization logic prevents broken links in the Monaco editor by removing problematic characters.
- The changes are consistent across all reference insertion methods.
- The implementation follows TypeScript best practices and coding guidelines.
The minor suggestions for readability (extracting string templates) and optimization (using a regex in the sanitization method) would further improve the code quality, but the current implementation is already solid and meets the requirements.
Great job on addressing this issue and improving the robustness of the reference insertion functionality!
src/test/javascript/spec/component/shared/monaco-editor/monaco-editor-communication-action.integration.spec.ts (1)
29-30: Imports are appropriateThe additions of
Attachmentanddayjsimports are necessary for the new test cases and are correctly implemented.
...t/spec/component/shared/monaco-editor/monaco-editor-communication-action.integration.spec.ts
Show resolved
Hide resolved
|
github-actions
bot
added
the
deployment-error
HawKhiem
added
deploy:artemis-test1
and removed
deployment-error
HawKhiem
temporarily deployed
to
artemis-test1.artemis.cit.tum.de
GitHub Actions
Inactive
Checklist
General
Client
Motivation and Context
Currently, it is possible to reference lectures, attachments, and attachment units that include brackets in their names. However, since round and square brackets are used for markdown syntax in the Monaco editor, their presence in names causes the generated links to break and become non-functional.
Description
I have implemented a function in the frontend that automatically removes both round and square brackets from reference titles and names. This ensures that users cannot inadvertently include brackets in their references, preventing potential issues with broken links.
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
Statements : 87.44% ( 55105/63015 )
Branches : 73.63% ( 20722/28142 )
Functions : 82.03% ( 12343/15046 )
Lines : 87.5% ( 52622/60136 )
Screenshots
Summary by CodeRabbit
New Features
Bug Fixes
Tests