Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lectures: Add hide/show functionality to PDF Preview Component #9667

Open
wants to merge 206 commits into
base: develop
Choose a base branch
from

Conversation

eceeeren
Copy link
Contributor

@eceeeren eceeeren commented Nov 4, 2024

Note: Please try to check for the PDF Preview view's functionalities again as Thumbnail Grid template is fixed from scratch :)

Checklist

General

Server

  • Important: I implemented the changes with a very good performance and prevented too many (unnecessary) and too complex database calls.
  • I strictly followed the principle of data economy for all database calls.
  • I strictly followed the server coding and design guidelines.
  • I added multiple integration tests (Spring) related to the features (with a high test coverage).
  • I added pre-authorization annotations according to the guidelines and checked the course groups for all new REST Calls (security).
  • I documented the Java code using JavaDoc style.

Client

  • Important: I implemented the changes with a very good performance, prevented too many (unnecessary) REST calls and made sure the UI is responsive, even with large data (e.g. using paging).
  • I strictly followed the principle of data economy for all client-server REST calls.
  • I strictly followed the client coding and design guidelines.
  • Following the theming guidelines, I specified colors only in the theming variable files and checked that the changes look consistent in both the light and the dark theme.
  • I added multiple integration tests (Jest) related to the features (with a high test coverage), while following the test guidelines.
  • I added authorities to all new routes and checked the course groups for displaying navigation elements (links, buttons).
  • I documented the TypeScript code using JSDoc style.
  • I added multiple screenshots/screencasts of my UI changes.
  • I translated all newly inserted strings into English and German.

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:

  1. Hide and Show buttons added for pages individually to able the slide hiding
  2. These buttons are shown once every page is hovered, to prevent a crowded view
  3. Once Hide Button is clicked, the page is grayed out and an "eye" icon is added to emphasise on being hidden
  4. When pages added, there are no changes as the new pages are always added at the end
  5. When some pages are deleted, even after the hidden pages are selected, previous hidden page indexes are updated accordingly
  6. When the file is saved, hidden pages indexes are saved in Slide table, therefore we can fetch them again when opening the view
  7. When the file is saved, a student version of the initial Attachment file is created and saved as studentVersion on the Attachment
  8. When navigated to Instructor view, the Instructor now can view the file as in Student Version (Download Button) and Original Version with the hidden files
  9. When navigated to Student View, the fetching mechanism is changed to create a Download button, as it can be either the original link or the hidden link
  10. When navigated to the Communication, when a student references a certain slide, the correct visible slide will be shown

Steps for Testing

Prerequisites:

  • 1 Instructor
  • 1 Student
  1. Log in to Artemis
  2. Navigate to Course Administration
  3. Go to 'Lecture Units'
  4. Create one Lecture Unit with a File
  5. Click "View" button
  6. Confirm that you can see "Hide" button when you hover over the pages individually
  7. Hide different pages and confirm that they are grayed out with "eye-slash" icon
  8. Add pages and delete some pages and ensure that initially hidden pages are not changed after the file is reloaded
  9. Save the file and confirm that you can see a success alert as "Attachment is updated"
  10. Return back to the previous page and confirm that the version is increased
  11. Confirm that you can see the "Download" button and "Original Version" button (only appear after some pages are hidden)
  12. Open the View page again and confirm that the pages you deleted are deleted and hidden pages are still hidden in the view
  13. Switch to a student user and navigate to the Course you created the Lecture Unit in
  14. Find the Lecture Unit you created and click Download
  15. The document should be opened in an external page and the deleted/hidden pages should not be shown and appended pages should be visible
  16. Navigate to Communication and add some slides as reference
  17. Make sure that a hidden slide is not shown and the next available visible slide is shown instead
  18. Get back to the Instructor user, create an attachment on the Attachment and Attachment Units sections and verify every feature is still working correctly

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

  • I (as a reviewer) confirm that the client changes (in particular related to REST calls and UI responsiveness) are implemented with a very good performance even for very large courses with more than 2000 students.
  • I (as a reviewer) confirm that the server changes (in particular related to database calls) are implemented with a very good performance even for very large courses with more than 2000 students.

Code Review

  • Code Review 1
  • Code Review 2

Manual Tests

  • Test 1
  • Test 2

Test Coverage

Class/File Line Coverage Confirmation (assert/expect)
attachment-unit.component.ts 94%
pdf-preview.component.ts 94%
pdf-preview-thumbnail-grid.component.ts 90%
lecture-unit.component.ts 100%
attachment-unit.service.spec.ts 100%

Screenshots

1. Hide Button on Hover
Screenshot 2024-11-26 at 14 48 55

2. Hidden Pages & Show Button on Hover
Screenshot 2024-11-26 at 14 49 08

3. Original Version button

Screenshot 2024-12-29 at 00 28 37

Summary by CodeRabbit

Release Notes

  • New Features

    • Added support for student-specific versions of attachments.
    • Introduced ability to hide specific pages in PDF attachments.
    • Enhanced PDF preview with page visibility controls.
    • New method for filtering hidden pages in lecture units.
    • Added functionality to manage original versions of attachments.
  • Improvements

    • Refined access controls for attachment and slide management.
    • Added more granular file handling for lecture units.
    • Improved localization support for original version functionality.
  • Bug Fixes

    • Updated file path handling for student-specific attachments.
    • Corrected slide indexing and referencing mechanisms.
  • Technical Enhancements

    • Optimized database fetch strategies for attachment units.
    • Improved error handling in file and attachment services.

HawKhiem
HawKhiem previously approved these changes Jan 2, 2025
Copy link

@HawKhiem HawKhiem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested again on TS6. Reapprove

@eceeeren eceeeren dismissed stale reviews from HawKhiem and coderabbitai[bot] via d0938ac January 6, 2025 17:30
coderabbitai[bot]
coderabbitai bot previously approved these changes Jan 6, 2025
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

♻️ Duplicate comments (2)
src/main/java/de/tum/cit/aet/artemis/lecture/service/AttachmentUnitService.java (2)

110-116: 🛠️ Refactor suggestion

Consider using a DTO for better parameter handling

The method signature has grown to 7 parameters, which violates the principle of method parameter count (ideally ≤ 3). Additionally, using String for hiddenPages is 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
) {}

209-223: 🛠️ Refactor suggestion

Improve robustness of student version handling

The method needs several improvements:

  1. Add input validation
  2. Extract hardcoded values
  3. Add error handling for file operations

Additionally, consider adding file type validation:

private void validateStudentVersionFile(MultipartFile file) {
    if (file.isEmpty()) {
        throw new BadRequestAlertException("Student version file cannot be empty", "AttachmentUnit", "emptyFile");
    }
    String extension = FilenameUtils.getExtension(file.getOriginalFilename());
    if (!"pdf".equals(extension)) {
        throw new BadRequestAlertException("Only PDF files are supported", "AttachmentUnit", "invalidFileType");
    }
}
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d0938ac and eed82fe.

📒 Files selected for processing (1)
  • src/main/java/de/tum/cit/aet/artemis/lecture/service/AttachmentUnitService.java (5 hunks)
🧰 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

⏰ Context from checks skipped due to timeout of 90000ms (8)
  • GitHub Check: client-tests-selected
  • GitHub Check: client-tests
  • GitHub Check: client-style
  • GitHub Check: server-tests
  • GitHub Check: server-style
  • GitHub Check: Build and Push Docker Image
  • GitHub Check: Build .war artifact
  • GitHub Check: Analyse
🔇 Additional comments (1)
src/main/java/de/tum/cit/aet/artemis/lecture/service/AttachmentUnitService.java (1)

131-133: 🛠️ Refactor suggestion

Add transaction boundary and error handling

The file operations should be wrapped in a transaction to ensure atomicity. Additionally, error handling should be added for both file operations.

+@Transactional
+try {
     updateAttachment(existingAttachment, updateAttachment, savedAttachmentUnit, hiddenPages);
     handleFile(updateFile, existingAttachment, keepFilename, savedAttachmentUnit.getId());
     handleStudentVersionFile(studentVersionFile, existingAttachment, savedAttachmentUnit.getId());
+} catch (Exception e) {
+    throw new BadRequestAlertException("Failed to process files: " + e.getMessage(), "AttachmentUnit", "fileProcessingError");
+}

Likely invalid or redundant comment.

Copy link

@kevinfischer4 kevinfischer4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested on TS2. The hiding/showing functionality works as expected on both sides (student and instructor).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client Pull requests that update TypeScript code. (Added Automatically!) core Pull requests that affect the corresponding module database Pull requests that update the database. (Added Automatically!). Require a CRITICAL deployment. lecture Pull requests that affect the corresponding module ready for review server Pull requests that update Java code. (Added Automatically!) tests
Projects
Status: Ready For Review
Development

Successfully merging this pull request may close these issues.

5 participants