Lectures: Add hide/show functionality to PDF Preview Component
#9667
Conversation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/test/javascript/spec/component/lecture/pdf-preview/pdf-preview.component.spec.ts (2)
210-242: Consider adding edge cases to strengthen test coverage.The test suite covers basic scenarios well, but could benefit from additional edge cases:
- Non-numeric page numbers
- Duplicate page numbers
- Very large page numbers
Add these test cases:
it('should handle non-numeric page numbers', () => { document.body.innerHTML = ` <button id="hide-show-button-abc" class="hide-show-btn btn-success"></button> `; const hiddenPages = component.getHiddenPages(); expect(hiddenPages).toEqual([]); }); it('should handle duplicate page numbers', () => { document.body.innerHTML = ` <button id="hide-show-button-1" class="hide-show-btn btn-success"></button> <button id="hide-show-button-1" class="hide-show-btn btn-success"></button> `; const hiddenPages = component.getHiddenPages(); expect(hiddenPages).toEqual([1]); });
475-532: Add test case for empty hidden pages array.The test suite should verify that the student version creation works correctly when no pages are hidden.
Add this test case:
it('should return original PDF when no pages are hidden', async () => { const hiddenPages: number[] = []; const mockPdfBytes = new Uint8Array([1, 2, 3, 4]).buffer; const mockFileName = 'test-file'; const mockAttachmentUnit = { attachment: { name: mockFileName, }, }; component.attachmentUnit.set(mockAttachmentUnit as any); component.currentPdfBlob.set(new Blob([mockPdfBytes], { type: 'application/pdf' })); component.currentPdfBlob()!.arrayBuffer = jest.fn().mockResolvedValue(mockPdfBytes); const result = await component.createStudentVersionOfAttachment(hiddenPages); expect(result).toBeInstanceOf(File); expect(result!.name).toBe(`${mockFileName}.pdf`); expect(result!.type).toBe('application/pdf'); expect(PDFDocument.load).toHaveBeenCalledTimes(1); expect(PDFDocument.prototype.removePage).not.toHaveBeenCalled(); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/test/javascript/spec/component/lecture/pdf-preview/pdf-preview.component.spec.ts(9 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/test/javascript/spec/component/lecture/pdf-preview/pdf-preview.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/lecture/pdf-preview/pdf-preview.component.spec.ts
Show resolved
Hide resolved
src/test/javascript/spec/component/lecture/pdf-preview/pdf-preview.component.spec.ts
Outdated
Show resolved
Hide resolved
… into feature/lectures/hide-pdf-pages
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/test/javascript/spec/component/lecture-unit/lecture-unit/lecture-unit.component.spec.ts (1)
84-99: Enhance test coverage for original version view.Good practices observed:
- Proper setup of inputs
- Button existence verification
- Consistent event handling
- Comprehensive spy assertions
Consider adding these test cases:
- Verify button is not shown when
showOriginalVersionButtonis false- Add specific assertions for emitted event data
it('should handle original version view', async () => { const handleOriginalVersionViewSpy = jest.spyOn(component, 'handleOriginalVersionView'); const onShowOriginalVersionEmitSpy = jest.spyOn(component.onShowOriginalVersion, 'emit'); + // Test when button should not be shown + fixture.componentRef.setInput('showOriginalVersionButton', false); + fixture.detectChanges(); + expect(fixture.debugElement.query(By.css('#view-original-version-button'))).toBeNull(); + + // Test when button should be shown fixture.componentRef.setInput('showOriginalVersionButton', true); fixture.detectChanges(); const event = new MouseEvent('click'); const button = fixture.debugElement.query(By.css('#view-original-version-button')); expect(button).not.toBeNull(); button.nativeElement.dispatchEvent(event); expect(handleOriginalVersionViewSpy).toHaveBeenCalledOnce(); - expect(onShowOriginalVersionEmitSpy).toHaveBeenCalledOnce(); + expect(onShowOriginalVersionEmitSpy).toHaveBeenCalledExactlyOnceWith(component.lectureUnit); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/main/resources/config/liquibase/master.xmlis excluded by!**/*.xml
📒 Files selected for processing (1)
src/test/javascript/spec/component/lecture-unit/lecture-unit/lecture-unit.component.spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/test/javascript/spec/component/lecture-unit/lecture-unit/lecture-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/lecture-unit/lecture-unit.component.spec.ts (1)
Line range hint 1-99: Add test coverage for PDF Preview hide/show functionality.
Based on the PR objectives, the following test scenarios appear to be missing:
- Hide/show buttons visibility on page hover
- Page hiding/showing functionality
- Hidden page state persistence
- Visual state of hidden pages (grayed out, eye icon)
src/test/javascript/spec/component/lecture-unit/lecture-unit/lecture-unit.component.spec.ts
Outdated
Show resolved
Hide resolved
src/test/javascript/spec/component/lecture-unit/lecture-unit/lecture-unit.component.spec.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/test/javascript/spec/component/lecture/pdf-preview/pdf-preview.component.spec.ts (1)
Line range hint
274-319: Add test coverage for hidden pages in attachment unit update.The test suite covers the basic update functionality but misses testing the interaction with hidden pages.
Add this test case:
+it('should update attachment unit successfully with hidden pages', fakeAsync(() => { + const hiddenPages = [1, 3]; + jest.spyOn(component, 'getHiddenPages').mockReturnValue(hiddenPages); + jest.spyOn(FormData.prototype, 'append'); + + component.attachment.set(undefined); + component.attachmentUnit.set({ + id: 1, + lecture: { id: 1 }, + attachment: { id: 1, version: 1 }, + }); + attachmentUnitServiceMock.update.mockReturnValue(of({})); + + component.updateAttachmentWithFile(); + tick(); + + const formData = new FormData(); + expect(FormData.prototype.append).toHaveBeenCalledWith('hiddenSlides', JSON.stringify(hiddenPages)); + expect(attachmentUnitServiceMock.update).toHaveBeenCalledWith(1, 1, expect.any(FormData)); +}));
🧹 Nitpick comments (3)
src/test/javascript/spec/component/lecture-unit/attachment-unit/attachment-unit.service.spec.ts (2)
228-234: Improve subscription callback pattern.The assignment in the subscription callback can be refactored to follow a more idiomatic pattern.
-let actualResponse: number[] | undefined; -service - .getHiddenSlides(lectureId, attachmentUnitId) - .pipe(take(1)) - .subscribe((response) => (actualResponse = response)); +service + .getHiddenSlides(lectureId, attachmentUnitId) + .pipe(take(1)) + .subscribe({ + next: (response) => { + expect(response).toEqual(expectedHiddenSlides); + } + });🧰 Tools
🪛 Biome (1.9.4)
[error] 233-233: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
224-242: Consider adding edge cases and error scenarios.The test covers the happy path but could benefit from additional test cases:
- Empty array response
- Error response handling
- Invalid lecture/attachment unit IDs
Example additional test case:
it('should handle empty hidden slides array', fakeAsync(() => { const lectureId = 1; const attachmentUnitId = 42; const expectedHiddenSlides: number[] = []; service .getHiddenSlides(lectureId, attachmentUnitId) .pipe(take(1)) .subscribe({ next: (response) => { expect(response).toEqual(expectedHiddenSlides); } }); const req = httpMock.expectOne({ url: `api/lectures/${lectureId}/attachment-units/${attachmentUnitId}/hidden-slides`, method: 'GET', }); req.flush(expectedHiddenSlides); }));🧰 Tools
🪛 Biome (1.9.4)
[error] 233-233: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
src/test/javascript/spec/component/lecture/pdf-preview/pdf-preview.component.spec.ts (1)
128-136: Enhance async test coverage with additional assertions.While the async test modifications correctly use
fakeAsyncandtick, consider adding assertions for the component's loading state.Add these assertions to verify the loading state:
it('should load attachment unit file and verify service calls when attachment unit data is available', fakeAsync(() => { routeMock.data = of({ course: { id: 1, name: 'Example Course' }, attachmentUnit: { id: 1, name: 'Chapter 1', lecture: { id: 1 } }, }); + expect(component.isLoading()).toBeTrue(); component.ngOnInit(); tick(); expect(attachmentUnitServiceMock.getAttachmentFile).toHaveBeenCalledWith(1, 1); + expect(component.isLoading()).toBeFalse(); }));Also applies to: 155-174, 184-195
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/main/resources/config/liquibase/master.xmlis excluded by!**/*.xml
📒 Files selected for processing (5)
src/main/java/de/tum/cit/aet/artemis/core/service/FilePathService.java(2 hunks)src/test/javascript/spec/component/lecture-unit/attachment-unit/attachment-unit.service.spec.ts(1 hunks)src/test/javascript/spec/component/lecture-unit/lecture-unit/lecture-unit.component.spec.ts(1 hunks)src/test/javascript/spec/component/lecture/pdf-preview/pdf-preview.component.spec.ts(10 hunks)src/test/javascript/spec/component/shared/http/file.service.spec.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/test/javascript/spec/component/lecture-unit/lecture-unit/lecture-unit.component.spec.ts
- src/main/java/de/tum/cit/aet/artemis/core/service/FilePathService.java
🧰 Additional context used
📓 Path-based instructions (3)
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/javascript/spec/component/lecture-unit/attachment-unit/attachment-unit.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/javascript/spec/component/lecture/pdf-preview/pdf-preview.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}}
🪛 Biome (1.9.4)
src/test/javascript/spec/component/lecture-unit/attachment-unit/attachment-unit.service.spec.ts
[error] 233-233: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
🔇 Additional comments (5)
src/test/javascript/spec/component/lecture-unit/attachment-unit/attachment-unit.service.spec.ts (1)
235-239: LGTM! HTTP mock configuration is correct.
The HTTP mock is properly configured:
- Correct URL pattern following REST conventions
- Appropriate HTTP method verification
- Proper request matching
src/test/javascript/spec/component/lecture/pdf-preview/pdf-preview.component.spec.ts (3)
3-3: LGTM: Mock setup is properly configured.
The addition of fakeAsync and tick imports, along with the getHiddenSlides mock implementation, aligns well with the asynchronous nature of the component's functionality.
Also applies to: 75-75
210-242: LGTM: Comprehensive test coverage for hidden pages functionality.
The test suite thoroughly covers various scenarios for the getHiddenPages method:
- Valid hidden page numbers
- Empty result when no matching elements exist
- Proper handling of invalid IDs
474-531: LGTM: Thorough test coverage for student version creation.
The test suite effectively covers:
- Successful creation of student version with hidden pages removed
- Error handling
- File naming and type verification
src/test/javascript/spec/component/shared/http/file.service.spec.ts (1)
235-243: Verify implementation matches test expectations.
Let's verify that the implementation in file.service.ts correctly handles all the test cases.
✅ Verification successful
Implementation matches test expectations
The implementation in src/main/webapp/app/shared/http/file.service.ts correctly handles the test case by:
- Finding the last slash in the URL using
lastIndexOf('/') - Inserting 'student' before the filename by concatenating the path up to the last slash with '/student' and the remaining filename
The implementation is also being used correctly in the codebase, as seen in attachment-unit.component.ts where it's used as a fallback when a student version is not explicitly available.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the implementation of createStudentLink method
# Find and display the implementation
echo "Searching for createStudentLink implementation..."
ast-grep --pattern 'createStudentLink($_) {
$$$
}'
# Check for URL handling utilities
echo "Checking for URL handling utilities..."
rg -A 5 "import.*URL" src/main/webapp/app/shared/http/file.service.ts
Length of output: 322
Script:
#!/bin/bash
# Let's try a different approach to find the implementation
# First, let's check if the file exists
echo "Checking if file.service.ts exists..."
fd "file.service.ts" src/main/webapp/app/shared/http/
# If it exists, let's look at its content
echo "Looking for createStudentLink in the implementation..."
rg -B 2 -A 5 "createStudentLink" src/main/webapp/app/shared/http/file.service.ts
# As a fallback, let's check the entire codebase for this method
echo "Searching entire codebase for createStudentLink..."
rg -B 2 -A 5 "createStudentLink" --type ts
Length of output: 6129
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/test/javascript/spec/component/lecture/pdf-preview/pdf-preview.component.spec.ts (3)
Line range hint
128-174: Enhance test readability with proper async handling.The async test cases follow good practices by using
fakeAsyncandtick(). However, consider adding more specific error expectations.it('should handle errors and trigger alert when loading an attachment unit file fails', fakeAsync(() => { // ... existing setup ... const alertServiceSpy = jest.spyOn(alertServiceMock, 'error'); component.ngOnInit(); tick(); - expect(alertServiceSpy).toHaveBeenCalled(); + expect(alertServiceSpy).toHaveBeenCalledWith( + 'artemisApp.attachment.pdfPreview.fileLoadError', + { error: 'File not found' } + ); }));
Line range hint
321-345: Add verification for FormData content.While the test verifies FormData.append calls, it should also verify the actual content being appended.
expect(FormData.prototype.append).toHaveBeenCalledWith('file', expect.any(File)); +const formData = new FormData(); +const appendSpy = jest.spyOn(FormData.prototype, 'append'); +const appendCalls = appendSpy.mock.calls; +expect(appendCalls[1][1]).toEqual(JSON.stringify({ + id: 1, + version: 1 +}));
347-366: Improve error handling test specificity.The error handling test should verify the exact error message and ensure proper cleanup.
it('should handle errors when updating an attachment unit fails', fakeAsync(() => { // ... existing setup ... + const errorMessage = 'Update failed'; - attachmentUnitServiceMock.update = jest.fn().mockReturnValue(throwError(() => new Error('Update failed'))); + attachmentUnitServiceMock.update = jest.fn().mockReturnValue(throwError(() => new Error(errorMessage))); component.updateAttachmentWithFile(); tick(); expect(attachmentUnitServiceMock.update).toHaveBeenCalledWith(1, 1, expect.any(FormData)); - expect(alertServiceMock.error).toHaveBeenCalledWith('artemisApp.attachment.pdfPreview.attachmentUpdateError', { error: 'Update failed' }); + expect(alertServiceMock.error).toHaveBeenCalledWith('artemisApp.attachment.pdfPreview.attachmentUpdateError', { error: errorMessage }); + expect(component.isPdfLoading()).toBeFalsy(); }));
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/webapp/i18n/de/lectureUnit.json(1 hunks)src/test/javascript/spec/component/lecture/pdf-preview/pdf-preview.component.spec.ts(10 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/main/webapp/i18n/de/lectureUnit.json (1)
Pattern src/main/webapp/i18n/de/**/*.json: German language translations should be informal (dutzen) and should never be formal (sietzen). So the user should always be addressed with "du/dein" and never with "sie/ihr".
src/test/javascript/spec/component/lecture/pdf-preview/pdf-preview.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 (5)
src/main/webapp/i18n/de/lectureUnit.json (2)
31-31: LGTM: Translation follows German language guidelines.
The translation "Originalversion" for "originalVersion" is correctly capitalized and aligns with the PR's document version management feature.
31-31: Verify translation key usage.
Let's verify where this translation key is being used in the codebase to ensure proper integration.
✅ Verification successful
Translation key is properly integrated and used in the application
The translation key 'originalVersion' is correctly integrated and used in the application:
- Present in both English (
en) and German (de) translation files underlectureUnit.json - Actively used in the component template at
src/main/webapp/app/overview/course-lectures/lecture-unit/lecture-unit.component.html - Used with the correct translation path
artemisApp.textUnit.originalVersionin the template - Associated with a button that shows/hides based on certain conditions (
showOriginalVersionButton())
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find usages of the 'originalVersion' translation key in the codebase
# Search for translation key usage in TypeScript/JavaScript files
echo "Searching for translation key usage..."
rg "originalVersion" --type ts --type js
# Search for the same key in other language files to ensure consistency
echo "Checking other language files..."
fd -e json -x rg "originalVersion" {}
Length of output: 343
Script:
#!/bin/bash
# Let's search more thoroughly with context and check for any variations of the key
# Search for any mentions of 'originalVersion' with context
echo "Searching with context in all files..."
rg -C 3 "originalVersion"
# Search specifically in translation-related files
echo "Searching in i18n files..."
fd "i18n" --type directory -x rg -l "originalVersion" {}
# Check the component files that might use this translation
echo "Searching in component files..."
rg -C 3 "lectureUnit.*originalVersion" --type ts --type tsx
Length of output: 3174
src/test/javascript/spec/component/lecture/pdf-preview/pdf-preview.component.spec.ts (3)
3-3: LGTM: Mock setup is properly configured.
The addition of getHiddenSlides mock and its return value is correctly implemented. The mock follows Jest's best practices by returning an Observable with test data.
Also applies to: 75-75
257-289: LGTM: Comprehensive test coverage for hidden pages functionality.
The test cases for getHiddenPages thoroughly cover:
- Success case with valid button IDs
- Empty case with no matching elements
- Edge case with invalid IDs
521-578: LGTM: Thorough test coverage for student version creation.
The test suite for createStudentVersionOfAttachment is well-structured with:
- Proper setup of mock data and PDF manipulation
- Verification of page removal operations
- Error handling scenarios
- File type and name assertions
src/test/javascript/spec/component/lecture/pdf-preview/pdf-preview.component.spec.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/main/java/de/tum/cit/aet/artemis/lecture/service/AttachmentUnitService.java (1)
110-116: 🛠️ Refactor suggestionMethod signature needs improvement for maintainability
The method signature has grown to 7 parameters, which reduces maintainability and readability. Additionally, using
StringforhiddenPagesis not type-safe.Consider creating a dedicated DTO to encapsulate these parameters:
public record AttachmentUnitUpdateDTO( AttachmentUnit unit, Attachment attachment, MultipartFile file, MultipartFile studentVersionFile, boolean keepFilename, Set<Integer> hiddenPages // Using Set<Integer> for type safety ) {}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/de/tum/cit/aet/artemis/lecture/service/AttachmentUnitService.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/lecture/web/AttachmentUnitResource.java(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/de/tum/cit/aet/artemis/lecture/web/AttachmentUnitResource.java
🧰 Additional context used
📓 Path-based instructions (1)
src/main/java/de/tum/cit/aet/artemis/lecture/service/AttachmentUnitService.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
🔇 Additional comments (1)
src/main/java/de/tum/cit/aet/artemis/lecture/service/AttachmentUnitService.java (1)
154-154: Document and validate hiddenPages format
The hiddenPages parameter is passed directly to the splitter service without validation or format documentation.
- Add validation:
private void validateHiddenPages(String hiddenPages) {
if (hiddenPages != null && !hiddenPages.matches("^(\\d+,)*\\d+$")) {
throw new BadRequestAlertException("Invalid hidden pages format", "AttachmentUnit", "invalidFormat");
}
}- Add documentation in the method's Javadoc:
* @param hiddenPages The hidden pages of attachment unit.
+ * Format: Comma-separated list of page numbers (e.g., "1,3,5")
src/main/java/de/tum/cit/aet/artemis/lecture/service/AttachmentUnitService.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/lecture/service/AttachmentUnitService.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/main/java/de/tum/cit/aet/artemis/lecture/service/SlideSplitterService.java (2)
47-47: Consider usingLocalDate.MAXfor representing "forever"Instead of using a specific far-future date, consider using
LocalDate.MAXwhich is a more semantic representation of "forever" and avoids potential timezone or calendar system issues.-static final LocalDate FOREVER = LocalDate.of(9999, 12, 31); +static final LocalDate FOREVER = LocalDate.MAX;
126-132: Consider adding @transactional for atomic operationsThe slide creation/update logic looks good, but consider adding transaction management to ensure atomicity when updating multiple slides.
+@Transactional public void splitAttachmentUnitIntoSingleSlides(PDDocument document, AttachmentUnit attachmentUnit, String pdfFilename, String hiddenPages) {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/de/tum/cit/aet/artemis/lecture/service/SlideSplitterService.java(5 hunks)src/main/java/de/tum/cit/aet/artemis/lecture/web/AttachmentUnitResource.java(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/de/tum/cit/aet/artemis/lecture/web/AttachmentUnitResource.java
🧰 Additional context used
📓 Path-based instructions (1)
src/main/java/de/tum/cit/aet/artemis/lecture/service/SlideSplitterService.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
🔇 Additional comments (2)
src/main/java/de/tum/cit/aet/artemis/lecture/service/SlideSplitterService.java (2)
Line range hint 64-97: LGTM! Well-structured method overloads
The overloaded methods maintain backward compatibility while properly delegating to the new implementation. The documentation is clear and complete.
114-115: Add error handling for 'hiddenPages' parsing
The current implementation is vulnerable to NumberFormatException when parsing hiddenPages.
src/main/java/de/tum/cit/aet/artemis/lecture/service/SlideSplitterService.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
src/main/webapp/app/lecture/pdf-preview/pdf-preview-thumbnail-grid/pdf-preview-thumbnail-grid.component.ts (1)
Line range hint
62-95: Implement proper resource cleanup for PDF renderingThe PDF loading process should properly clean up resources to prevent memory leaks. Consider implementing cleanup in a destroy hook.
Add the following cleanup logic:
private pdfDocument?: PDFJS.PDFDocumentProxy; async loadPdf(fileUrl: string, append: boolean): Promise<void> { // Clean up previous PDF document if (this.pdfDocument) { await this.pdfDocument.destroy(); } try { const loadingTask = PDFJS.getDocument(fileUrl); this.pdfDocument = await loadingTask.promise; // ... rest of the code } // ... catch and finally blocks } ngOnDestroy(): void { if (this.pdfDocument) { this.pdfDocument.destroy().catch(console.error); } }
♻️ Duplicate comments (1)
src/main/webapp/app/lecture/pdf-preview/pdf-preview-thumbnail-grid/pdf-preview-thumbnail-grid.component.ts (1)
130-137:⚠️ Potential issueAvoid mutating input signals directly
The
toggleVisibilitymethod directly mutates thehiddenPagesinput signal, which violates Angular's unidirectional data flow principles.Apply this diff:
toggleVisibility(pageIndex: number, event: Event): void { - if (this.hiddenPages()!.has(pageIndex)) { - this.hiddenPages()!.delete(pageIndex); - } else { - this.hiddenPages()!.add(pageIndex); - } - this.newHiddenPagesOutput.emit(this.hiddenPages()!); + const updatedPages = new Set(this.newHiddenPages()); + if (updatedPages.has(pageIndex)) { + updatedPages.delete(pageIndex); + } else { + updatedPages.add(pageIndex); + } + this.newHiddenPages.set(updatedPages); + this.newHiddenPagesOutput.emit(updatedPages); event.stopPropagation(); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/webapp/app/lecture/pdf-preview/pdf-preview-enlarged-canvas/pdf-preview-enlarged-canvas.component.ts(5 hunks)src/main/webapp/app/lecture/pdf-preview/pdf-preview-thumbnail-grid/pdf-preview-thumbnail-grid.component.html(1 hunks)src/main/webapp/app/lecture/pdf-preview/pdf-preview-thumbnail-grid/pdf-preview-thumbnail-grid.component.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/webapp/app/lecture/pdf-preview/pdf-preview-enlarged-canvas/pdf-preview-enlarged-canvas.component.ts
- src/main/webapp/app/lecture/pdf-preview/pdf-preview-thumbnail-grid/pdf-preview-thumbnail-grid.component.html
🧰 Additional context used
📓 Path-based instructions (1)
src/main/webapp/app/lecture/pdf-preview/pdf-preview-thumbnail-grid/pdf-preview-thumbnail-grid.component.ts (1)
🔇 Additional comments (1)
src/main/webapp/app/lecture/pdf-preview/pdf-preview-thumbnail-grid/pdf-preview-thumbnail-grid.component.ts (1)
Line range hint 1-17: LGTM! Component setup follows Angular best practices
The component is properly configured as standalone with appropriate imports and modern Angular features.
| selectedPages = signal<Set<number>>(new Set()); | ||
| originalCanvas = signal<HTMLCanvasElement | undefined>(undefined); | ||
| newHiddenPages = signal(new Set<number>(this.hiddenPages()!)); |
There was a problem hiding this comment.
Add null safety check for hiddenPages initialization
The initialization of newHiddenPages assumes hiddenPages is non-null, which could lead to runtime errors.
Apply this diff:
-newHiddenPages = signal(new Set<number>(this.hiddenPages()!));
+newHiddenPages = signal(new Set<number>(this.hiddenPages() ?? new Set()));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| newHiddenPages = signal(new Set<number>(this.hiddenPages()!)); | |
| newHiddenPages = signal(new Set<number>(this.hiddenPages() ?? new Set())); |
| ngOnChanges(changes: SimpleChanges): void { | ||
| if (changes['hiddenPages']) { | ||
| this.newHiddenPages.set(new Set(this.hiddenPages()!)); | ||
| } | ||
| if (changes['currentPdfUrl']) { | ||
| this.loadPdf(this.currentPdfUrl()!, this.appendFile()!); | ||
| } |
There was a problem hiding this comment.
Add null checks in ngOnChanges
The loadPdf call assumes both currentPdfUrl and appendFile are non-null, which could lead to runtime errors.
Apply this diff:
-this.loadPdf(this.currentPdfUrl()!, this.appendFile()!);
+const url = this.currentPdfUrl();
+if (url) {
+ this.loadPdf(url, this.appendFile() ?? false);
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ngOnChanges(changes: SimpleChanges): void { | |
| if (changes['hiddenPages']) { | |
| this.newHiddenPages.set(new Set(this.hiddenPages()!)); | |
| } | |
| if (changes['currentPdfUrl']) { | |
| this.loadPdf(this.currentPdfUrl()!, this.appendFile()!); | |
| } | |
| ngOnChanges(changes: SimpleChanges): void { | |
| if (changes['hiddenPages']) { | |
| this.newHiddenPages.set(new Set(this.hiddenPages()!)); | |
| } | |
| if (changes['currentPdfUrl']) { | |
| const url = this.currentPdfUrl(); | |
| if (url) { | |
| this.loadPdf(url, this.appendFile() ?? false); | |
| } | |
| } |
| displayEnlargedCanvas(pageIndex: number): void { | ||
| const canvas = this.pdfContainer().nativeElement.querySelector(`#pdf-page-${pageIndex} canvas`) as HTMLCanvasElement; | ||
| this.originalCanvas.set(canvas!); | ||
| this.isEnlargedView.set(true); | ||
| this.initialPageNumber.set(pageIndex); | ||
| } |
There was a problem hiding this comment.
Add null check for canvas selection
The canvas selection assumes the element exists, which could lead to runtime errors if the DOM structure changes.
Apply this diff:
displayEnlargedCanvas(pageIndex: number): void {
const canvas = this.pdfContainer().nativeElement.querySelector(`#pdf-page-${pageIndex} canvas`) as HTMLCanvasElement;
- this.originalCanvas.set(canvas!);
+ if (!canvas) {
+ this.alertService.error('error.canvasNotFound');
+ return;
+ }
+ this.originalCanvas.set(canvas);
this.isEnlargedView.set(true);
this.initialPageNumber.set(pageIndex);
}Committable suggestion skipped: line range outside the PR's diff.
Note: Please try to check for the PDF Preview view's functionalities again as Thumbnail Grid template is fixed from scratch :)
Checklist
General
Server
Client
authoritiesto all new routes and checked the course groups for displaying navigation elements (links, buttons).Motivation and Context
This PR introduces a feature enabling the selective visibility of specific pages within a PDF document. Instructors often require the ability to retain certain pages in a document while temporarily withholding their visibility, for instance, until the conclusion of an exercise submission period. Currently, achieving this functionality necessitates dividing the content into multiple files, which can be cumbersome and inefficient. To address this limitation, a solution has been proposed to allow instructors to dynamically hide or reveal individual pages directly within the PDF Preview Component, enhancing the flexibility and usability of the system.
Description
A solution is implemented in PDF Preview Component, such as:
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
Performance Review
Code Review
Manual Tests
Test Coverage
Screenshots
1. Hide Button on Hover
2. Hidden Pages & Show Button on Hover
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Tests