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 attachment file upload #6569

Merged
merged 17 commits into from
May 27, 2023

Conversation

julian-christl
Copy link
Member

@julian-christl julian-christl commented May 12, 2023

Checklist

General

Server

  • Important: I implemented the changes with a very good performance and prevented too many (unnecessary) database calls.
  • I followed the coding and design guidelines.
  • I added multiple integration tests (Spring) related to the features (with a high test coverage).
  • 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.
  • I followed the coding and design guidelines.
  • 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

Follow-up of #5733 and #5427 to replace the file upload before entity persistence with a multipart file and entity upload.

Description

I simply extended the endpoints to save an attachment to also allow a file upload (forced for creation), save one REST call and make the system safer. No UI changes were necessary..

Steps for Testing

Prerequisites:

  • 1 Instructor
  • 1 Student
  1. Create a lecture in a course
  2. Navigate to the "Attachments" of this lecture (not the attachment units!)
  • Create an attachment
  • Update the attachment with a new file
  • Update the attachment without a new file
  • Make sure that the student can see the file and the changes correctly

Review Progress

Performance Review

  • I confirm that the client changes (in particular related to REST calls and UI responsiveness) are implemented with a very good performance
  • I confirm that the server changes (in particular related to database calls) are implemented with a very good performance

Code Review

  • Code Review 1
  • Code Review 2

Manual Tests

  • Test 1
  • Test 2

Test Coverage

I added assertions where needed. However, for most of the changes, the existing assertions were already enough.

Class/File Line Coverage Confirmation (assert/expect)
AttachmentResource.java 98.1%
attachment.service.ts 95.9%
lecture-attachments.component.ts 99%

@julian-christl julian-christl self-assigned this May 12, 2023
@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 May 12, 2023
@artemis-bot artemis-bot added this to In progress in Artemis Development May 12, 2023
@julian-christl julian-christl changed the title Feature/reimplement attachment upload Attachment: Reimplement file upload May 12, 2023
@julian-christl julian-christl force-pushed the feature/reimplement-attachment-upload branch from c5ccbb7 to a016316 Compare May 12, 2023 09:05
@github-actions
Copy link

There hasn't been any activity on this pull request recently. Therefore, this pull request has been automatically marked as stale and will be closed if no further activity occurs within seven days. Thank you for your contributions.

@github-actions github-actions bot added the stale label May 19, 2023
@julian-christl julian-christl force-pushed the feature/reimplement-attachment-upload branch 3 times, most recently from ff9cd42 to 1cfaf5c Compare May 19, 2023 22:43
@codecov-commenter
Copy link

codecov-commenter commented May 19, 2023

Codecov Report

Merging #6569 (19acb9c) into develop (5ac110a) will decrease coverage by 0.01%.
The diff coverage is 82.65%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files
@@              Coverage Diff              @@
##             develop    #6569      +/-   ##
=============================================
- Coverage      80.39%   80.38%   -0.01%     
+ Complexity     13588    13587       -1     
=============================================
  Files           2398     2399       +1     
  Lines          91684    91692       +8     
  Branches       12870    12871       +1     
=============================================
+ Hits           73706    73710       +4     
- Misses          9886     9888       +2     
- Partials        8092     8094       +2     
Impacted Files Coverage Δ
.../java/de/tum/in/www1/artemis/config/Constants.java 100.00% <ø> (ø)
...m/in/www1/artemis/web/rest/CompetencyResource.java 95.02% <0.00%> (ø)
...ebapp/app/course/manage/course-update.component.ts 74.43% <50.00%> (-0.79%) ⬇️
...ain/java/de/tum/in/www1/artemis/domain/Course.java 85.77% <66.66%> (-0.82%) ⬇️
src/main/webapp/app/lecture/attachment.service.ts 83.67% <77.77%> (-0.05%) ⬇️
...ww1/artemis/service/AuthorizationCheckService.java 75.57% <82.35%> (+0.37%) ⬆️
...ebapp/app/lecture/lecture-attachments.component.ts 88.46% <86.95%> (+2.62%) ⬆️
...e/tum/in/www1/artemis/web/rest/CourseResource.java 90.78% <92.30%> (ø)
.../www1/artemis/repository/AttachmentRepository.java 100.00% <100.00%> (ø)
...m/in/www1/artemis/repository/CourseRepository.java 92.68% <100.00%> (ø)
... and 5 more

... and 4 files with indirect coverage changes

Flag Coverage Δ
client 76.12% <78.57%> (-0.01%) ⬇️
server 85.05% <85.71%> (-0.01%) ⬇️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0532ade...19acb9c. Read the comment docs.

@github-actions github-actions bot removed the stale label May 20, 2023
@julian-christl julian-christl force-pushed the feature/reimplement-attachment-upload branch from 1cfaf5c to cc7ba45 Compare May 23, 2023 14:26
@julian-christl julian-christl marked this pull request as ready for review May 23, 2023 15:15
@julian-christl julian-christl requested a review from a team as a code owner May 23, 2023 15:15
@artemis-bot artemis-bot moved this from In progress to Ready for review in Artemis Development May 23, 2023
@artemis-bot artemis-bot moved this from Ready for review to In progress in Artemis Development May 23, 2023
@SolizerCodes SolizerCodes temporarily deployed to artemis-test5.artemis.cit.tum.de May 25, 2023 18:51 — with GitHub Actions Inactive
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.

Reapprove code

Copy link
Contributor

@DominikRemo DominikRemo left a comment

Choose a reason for hiding this comment

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

Reapproved

Copy link
Contributor

@tunargs tunargs left a comment

Choose a reason for hiding this comment

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

I tested it on ts1, works perfectly. The code looks good as well.

Copy link
Contributor

@MarkusPaulsen MarkusPaulsen left a comment

Choose a reason for hiding this comment

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

Code looks good and tested on ts1.

Copy link
Contributor

@dearjasmina dearjasmina 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, works as expected
Code looks great as well :)

Artemis Development automation moved this from Review in progress to Reviewer approved May 26, 2023
@krusche krusche modified the milestones: 6.2.0, 6.2.1 May 26, 2023
@bassner bassner changed the title Attachment: Reimplement file upload Lectures: Reimplement file upload May 27, 2023
@bassner bassner changed the title Lectures: Reimplement file upload Lectures: Reimplement attachment file upload May 27, 2023
@bassner bassner merged commit d32b29a into develop May 27, 2023
20 of 27 checks passed
@bassner bassner deleted the feature/reimplement-attachment-upload branch May 27, 2023 07:57
@bassner bassner modified the milestones: 6.2.1, 6.2.0 May 27, 2023
@krusche krusche changed the title Lectures: Reimplement attachment file upload Development: Reimplement attachment file upload May 27, 2023
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!) enhancement ready to merge refactoring server Pull requests that update Java code. (Added Automatically!) tests
Projects
No open projects
Artemis Development
  
Reviewer approved
Development

Successfully merging this pull request may close these issues.

None yet