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: Improve the file upload for attachments #5427

Merged
merged 31 commits into from
Sep 16, 2022

Conversation

julian-christl
Copy link
Member

@julian-christl julian-christl commented Jul 18, 2022

Checklist

General

Server

  • I followed the coding and design guidelines.
  • I added multiple integration tests (Spring) related to the features (with a high test coverage).
  • I added @PreAuthorize and checked the course groups for all new REST Calls (security).
  • I implemented the changes with a good performance and prevented too many database calls.
  • I documented the Java code using JavaDoc style.

Client

  • I added multiple integration tests (Jest) related to the features (with a high test coverage), while following the test guidelines.
  • I documented the TypeScript code using JSDoc style.

Motivation and Context

As part of my bachelor's thesis, I'm replacing the file upload occurrences with a more secure way.
The first occurrences are the attachments that I'm addressing in this PR.

Description

  • Deprecated the file upload endpoint as we want to replace it
  • Moved code from the FileResource to the FileService
  • Replaced the simple endpoint to create and update attachment units with similar endpoints that take everything in a multipart upload, including attachment unit, attachment and file
  • In the client, I replaced old occurrences where we used multiple calls to the server with the new unified call, extended the blob-util.ts for this
  • I added tests for the blob-util.ts
  • I removed the legacy code from the blob-utils.ts because all browsers we support (.browserlistrc) support the blob constructor

Steps for Testing

Prerequisites:

  • 1 Instructor
  • 1 Student

Note: While creating and editing, keep track with your student account if everything works out from the student's point of view (especially the download of the files)

  1. Create a lecture
  2. Create a file lecture unit
  3. Update the file lecture unit with full changes
  4. Update the file lecture unit gradually (with and without a new file)
  5. Create lecture units of other types and make sure nothing is broken

Review Progress

Code Review

  • Review 1
  • Review 2

Manual Tests

  • Test 1
  • Test 2

Test Coverage

Class/File Branch Line
AttachmentUnitService.java 70% 98%
FileService.java 85% 87.41%
AttachmentUnitResource.java 85% 98%
FileResource.java 77% 88.46%
attachment-unit-form.component.ts 77.58% 91.11%
attachmentUnit.service.ts 70% 100%
create-attachment-unit.component.ts 76.59% 95.34%
edit-attachment-unit.component.ts 77.77% 95.45%
blob-util.ts 88.88% 100%

@julian-christl julian-christl requested a review from a team as a code owner July 18, 2022 20:50
@artemis-bot artemis-bot added this to In progress in Artemis Development Jul 18, 2022
@github-actions github-actions bot added client Pull requests that update TypeScript code. (Added Automatically!) server Pull requests that update Java code. (Added Automatically!) tests labels Jul 18, 2022
@julian-christl julian-christl changed the title File Upload: Replace file upload attachments with a single request File Upload: Replace file upload attachments with a single request Jul 18, 2022
@julian-christl julian-christl marked this pull request as draft July 18, 2022 23:17
@julian-christl julian-christl force-pushed the feature/replace-file-upload-for-attachments branch 2 times, most recently from a196501 to d8ddc9c Compare August 7, 2022 13:59
@julian-christl julian-christl force-pushed the feature/replace-file-upload-for-attachments branch from bb7d207 to bece305 Compare August 14, 2022 14:34
@julian-christl julian-christl marked this pull request as ready for review August 14, 2022 18:45
@artemis-bot artemis-bot moved this from In progress to Ready for review in Artemis Development Aug 14, 2022
@julian-christl julian-christl requested a review from a team August 14, 2022 22:06
b-fein
b-fein previously requested changes Aug 15, 2022
Copy link
Contributor

@b-fein b-fein left a comment

Choose a reason for hiding this comment

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

Initial pass over the server code, no in-depth code-review yet.

Artemis Development automation moved this from Ready for review to Review in progress Aug 15, 2022
@julian-christl julian-christl dismissed b-fein’s stale review August 19, 2022 14:09

implemented the change requests

@julian-christl julian-christl force-pushed the feature/replace-file-upload-for-attachments branch from 61a618c to b20c8ca Compare August 19, 2022 14:17
@julian-christl julian-christl temporarily deployed to artemistest5 August 19, 2022 18:43 Inactive
Copy link
Contributor

@b-fein b-fein left a comment

Choose a reason for hiding this comment

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

Code re-approve

Copy link
Contributor

@ge65cer ge65cer left a comment

Choose a reason for hiding this comment

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

Thanks for the changes, approved.

Copy link
Contributor

@JohannesStoehr JohannesStoehr left a comment

Choose a reason for hiding this comment

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

Refactoring looks good

Copy link
Contributor

@stefanwaldhauser stefanwaldhauser left a comment

Choose a reason for hiding this comment

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

Reapprove

@julian-christl julian-christl temporarily deployed to artemistest5 September 14, 2022 13:22 Inactive
Copy link
Member

@bassner bassner left a comment

Choose a reason for hiding this comment

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

Approved 👍

@julian-christl julian-christl added this to the 5.10.3 milestone Sep 15, 2022
@krusche krusche changed the title File Upload: Replace file upload attachments with a single request Lecture: Replace file upload attachments with a single request Sep 16, 2022
@krusche krusche changed the title Lecture: Replace file upload attachments with a single request Lectures: Replace file upload attachments with a single request Sep 16, 2022
@krusche krusche changed the title Lectures: Replace file upload attachments with a single request Lectures: Improve the file upload for attachments Sep 16, 2022
@krusche krusche merged commit 2a17f53 into develop Sep 16, 2022
@krusche krusche deleted the feature/replace-file-upload-for-attachments branch September 16, 2022 15:05
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!) ready to merge server Pull requests that update Java code. (Added Automatically!) tests
Projects
No open projects
Artemis Development
  
Review in progress
Development

Successfully merging this pull request may close these issues.

None yet

7 participants