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

Development: Reimplement course icon file upload #5733

Merged
merged 21 commits into from
Nov 7, 2022

Conversation

julian-christl
Copy link
Member

@julian-christl julian-christl commented Oct 13, 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 followed the coding and design guidelines and ensured that the layout is responsive.
  • 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 documented the TypeScript code using JSDoc style.
  • I added multiple screenshots/screencasts of my UI changes.

Motivation and Context

Follows #5427 to reimplement all file uploads in our system in a uniform and secure way.

Description

  • Moved PUT /api/course to PUT /api/course/:id to reflect our standard. I, therefore, also removed the implementation to create a course with PUT. This is not used in our system and only complicates the implementation.
  • Modified the create and update endpoints of the course to receive an optional file and save it as the course icon
  • Removed the manual upload of the icon and replaced it with a streamlined upload when saving the course.
  • Adapted the implementation of detecting changes in the user groups
  • I implemented a workaround in the cypress tests because the response's body is an ArrayBuffer instead of an object. Such an issue is already documented (e.g. intercept() sometimes parses request or response body as ArrayBuffer cypress-io/cypress#17055 for intercept()). Therefore, I implemented a fix that will fail once this gets fixed by Cypress. We can then remove the workaround.

Steps for Testing

Prerequisites:

  • 1 Admin
  • 1 Instructor
  1. Create Course A with an Admin with a custom image
  2. Create Course B with an Admin without a custom image
  3. Update a course with an admin adapting the image (expect image updated)
  4. Update a course with an admin adapting the user groups (expect groups updated)
  5. Update a course with an instructor adapting the image (expect image updated)
  6. Update a course with an instructor adapting the image and the user group (you may have to use the API or enable the fields in the DOM. Expect error, no image update)

Review Progress

Code Review

  • Review 1
  • Review 2

Manual Tests

  • Test 1
  • Test 2

Test Coverage

Class/File Branch Line
CourseResource.java 72% 95.25%
course-management.service.ts 77.06% 90.21%
course-update.component.ts 70.24% 86.2%
file-uploader.service.ts 41.66% 53.84%
image-cropper.component.ts 33.5% 45.25%

Screenshots

Before

After

@julian-christl julian-christl self-assigned this Oct 13, 2022
@julian-christl julian-christl requested a review from a team as a code owner October 13, 2022 10:52
@artemis-bot artemis-bot added this to In progress in Artemis Development Oct 13, 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 Oct 13, 2022
@julian-christl julian-christl changed the title Course: Rreimplement course icon file upload Course: Reimplement course icon file upload Oct 13, 2022
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.

Tested on TS1 in the testing session.
When editing the course without uploading a new icon, I can click the save button, but the changes are not persisted (no request sent to the server).

Artemis Development automation moved this from In progress to Review in progress Oct 13, 2022
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.

#5691 adds the possibility to delete the course icon. Maybe you could talk to the other developer and find a unified solution for your both PRs

@julian-christl julian-christl marked this pull request as draft October 13, 2022 12:52
@artemis-bot artemis-bot moved this from Review in progress to In progress in Artemis Development Oct 13, 2022
@github-actions github-actions bot added the cypress Pull requests that update cypress tests. (Added Automatically!) label Oct 13, 2022
@julian-christl julian-christl force-pushed the feature/reimplement-course-image-upload branch from 512347d to 4c0db3f Compare October 13, 2022 17:51
@julian-christl julian-christl force-pushed the feature/reimplement-course-image-upload branch from 627442a to 36963d2 Compare October 13, 2022 20:02
@julian-christl julian-christl force-pushed the feature/reimplement-course-image-upload branch from 36963d2 to cf1ed38 Compare October 13, 2022 20:27
@julian-christl julian-christl force-pushed the feature/reimplement-course-image-upload branch from cf1ed38 to b6abe6e Compare October 13, 2022 20:55
@julian-christl julian-christl force-pushed the feature/reimplement-course-image-upload branch from b6abe6e to 9d721f7 Compare October 13, 2022 21:47
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.

Code lgtm. Gj!

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.

Checked the code changes again. Looks good to me

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

Copy link
Member

@rriyaldhi rriyaldhi left a comment

Choose a reason for hiding this comment

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

Reapprove

@krusche krusche changed the title Course: Reimplement course icon file upload Development: Reimplement course icon file upload Nov 7, 2022
@krusche krusche added this to the 5.12.3 milestone Nov 7, 2022
@krusche krusche merged commit 59f418e into develop Nov 7, 2022
@krusche krusche deleted the feature/reimplement-course-image-upload branch November 7, 2022 22:36
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!) cypress Pull requests that update cypress tests. (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

6 participants