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

Quiz exercises: Simplify drag-and-drop image upload #6456

Merged
merged 53 commits into from
Jun 27, 2023

Conversation

julian-christl
Copy link
Member

@julian-christl julian-christl commented Apr 18, 2023

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 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.
  • 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.
  • I added multiple screenshots/screencasts of my UI changes.
  • I translated all newly inserted strings into English and German.

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

The changes only affect the drag-and-drop section of the quizzes. But because the quiz editor allows creating all different types of quiz questions, I had to make the component saving the entity to retrieve the files from potential drag-and-drop question children and upload them together with the files.

It affects the Creation, Update, Re-evaluation and Import of quiz exercises. Additionally, it also affects the creation of drag-and-drop mode quizzes. However, those use the same creation endpoint.

I excluded one cypress tests that blocks this PR because it only fails on CI. It works locally and manual tests confirm that the functionality works. We try debugging and fixing this test in #6631 not further to block this PR.

Steps for Testing

We need to fully test the quiz functionalities, including:

  • Creation of a normal quiz
  • Creation of a Drag and Drop Model Quiz
  • Edit
  • Import
  • Reevaluation
  • Adding existing questions (bottom of the editor) to a quiz exercise

During all of these five checks, it makes sense to work

  • with and without dnd questions
  • with and without background
  • with and without picture drag and drop items
  • with and without text drag and drop items
  • with both drag and drop items
  • with adding, removing, replacing Drag Items and Questions to make sure the files get cleared up and not sent to the server

Always check if the quiz gets rendered as expected.

Exam Mode Testing

Most of the changes are unrelated to the exam mode. I do not think testing is necessary to test the exam mode as extensively. However, it makes sense to just test the normal quiz exam workflow (creation and conduction) to be sure.

The only change that does require testing here is the import functionality of an existing exercise.

Review Progress

Code Review

  • Review 1
  • Review 2

Manual Tests

  • Test 1
  • Test 2

Exam Mode Tests

  • Test 1
  • Test 2

Test Coverage

Class/File Line Coverage Confirmation (assert/expect)
FileService.java 87.0%
QuizExerciseImportService.java 93.7%
QuizExerciseService.java 97.7%
ExamImportService.java 95.5%
QuizExerciseResource.java 93.6%
quiz-exercise-generator.ts 89.6%
drag-and-drop-question-edit.component.ts 91.6%
quiz-exercise-detail.component.ts 92.8%
quiz-question-list-edit.component.ts 94.8%
quiz-question-list-edit-existing.component.ts 98.9%
re-evaluate-drag-and-drop-question.component.ts 100%
quiz-re-evaluate.component.ts 86.1%
quiz-exercise.service.ts 100%
quiz-re-evaluate.service.ts 100%
file.service.ts 63.0%

Screenshots

Upload background before (before changes):
1409d1ba0c797661134ab989845ec831
Upload Background before (after changes):
e050fc2202937de46c2976423242c873

Upload background after (before changes):
80251aeeb77554f93832507fce51ee06
Upload background after (after changes):
3557ddaac548194325b3defb284e7f34

Upload picture drag item after (before changes):
593948ae5992864d34e4832ef35d9341
Upload picture drag item after (after changes):
f7deccf1b245d6010fb76aa3a1abbff3

@julian-christl julian-christl self-assigned this Apr 18, 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 Apr 18, 2023
@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 Apr 26, 2023
@julian-christl julian-christl force-pushed the feature/reimplement-quiz-image-upload-2 branch from ea45b65 to 9b02255 Compare April 27, 2023 13:42
@julian-christl julian-christl force-pushed the feature/reimplement-quiz-image-upload-2 branch from 9b02255 to 88a1276 Compare April 27, 2023 16:02
@github-actions github-actions bot added the cypress Pull requests that update cypress tests. (Added Automatically!) label Apr 27, 2023
@julian-christl julian-christl force-pushed the feature/reimplement-quiz-image-upload-2 branch from 285f5ab to 57c682b Compare April 27, 2023 17:36
@codecov-commenter
Copy link

codecov-commenter commented Apr 27, 2023

Codecov Report

Merging #6456 (827f6d5) into develop (d5897fe) will decrease coverage by 4.54%.
The diff coverage is 81.67%.

❗ 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    #6456      +/-   ##
=============================================
- Coverage      84.96%   80.42%   -4.54%     
- Complexity     13912    13952      +40     
=============================================
  Files           1156     2448    +1292     
  Lines          44886    93506   +48620     
  Branches        4623    13137    +8514     
=============================================
+ Hits           38137    75203   +37066     
- Misses          4225    10055    +5830     
- Partials        2524     8248    +5724     
Impacted Files Coverage Δ
.../tum/in/www1/artemis/domain/quiz/QuizExercise.java 74.87% <0.00%> (ø)
.../de/tum/in/www1/artemis/web/rest/ExamResource.java 95.02% <ø> (ø)
...n/www1/artemis/web/rest/ExerciseGroupResource.java 91.52% <ø> (ø)
...ercises/quiz/manage/quiz-exercise-popup.service.ts 31.81% <0.00%> (ø)
.../re-evaluate/quiz-re-evaluate-warning.component.ts 38.38% <0.00%> (ø)
...in/webapp/app/shared/http/file-uploader.service.ts 42.85% <ø> (ø)
...z/manage/re-evaluate/quiz-re-evaluate.component.ts 78.48% <28.57%> (ø)
...n/www1/artemis/service/exam/ExamImportService.java 90.26% <57.14%> (+0.34%) ⬆️
.../www1/artemis/domain/quiz/DragAndDropQuestion.java 70.42% <63.63%> (-0.09%) ⬇️
...-question/drag-and-drop-question-edit.component.ts 78.18% <65.21%> (ø)
... and 17 more

... and 1281 files with indirect coverage changes

Flag Coverage Δ
client 76.19% <77.91%> (?)
server 84.98% <84.34%> (+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 d5897fe...827f6d5. Read the comment docs.

Copy link

@YeetTheFirst21 YeetTheFirst21 left a comment

Choose a reason for hiding this comment

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

Retested on TS1,

both normal quiz and exam mode seems to be working as expected with imports.

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.

Code looks good to me

@tobias-lippert tobias-lippert temporarily deployed to artemis-test4.artemis.cit.tum.de June 26, 2023 10:28 — with GitHub Actions Inactive
Copy link
Contributor

@tobias-lippert tobias-lippert left a comment

Choose a reason for hiding this comment

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

Retested on ts4. Everything still seems to work.

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.

reapprove

Copy link
Contributor

@Santia-go Santia-go left a comment

Choose a reason for hiding this comment

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

Re approved!
The last changes do not change the behavior previously tested.

@Santia-go Santia-go added the maintainer-approved The feature maintainer has approved the PR label Jun 27, 2023
@julian-christl julian-christl added this to the 6.3.2 milestone Jun 27, 2023
@bassner bassner changed the title Quiz exercises: Reimplement drag and drop image upload Development: Reimplement drag-and-drop image upload Jun 27, 2023
@bassner bassner merged commit d8b419a into develop Jun 27, 2023
29 of 38 checks passed
@bassner bassner deleted the feature/reimplement-quiz-image-upload-2 branch June 27, 2023 11:52
@julian-christl julian-christl changed the title Development: Reimplement drag-and-drop image upload Quiz exercises: Reimplement drag-and-drop image upload Jun 27, 2023
@julian-christl julian-christl changed the title Quiz exercises: Reimplement drag-and-drop image upload Quiz exercises: Simplify drag-and-drop image upload Jun 27, 2023
krusche added a commit that referenced this pull request Jul 6, 2023
This reverts commit d8b419a.

# Conflicts:
#	src/test/java/de/tum/in/www1/artemis/exercise/quizexercise/QuizExerciseIntegrationTest.java
@Strohgelaender Strohgelaender restored the feature/reimplement-quiz-image-upload-2 branch July 7, 2023 10:54
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!) enhancement maintainer-approved The feature maintainer has approved the PR ready to merge refactoring server Pull requests that update Java code. (Added Automatically!) tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.