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: Use default value if exercise has no plagiarism checks config #7665

Conversation

jakubriegel
Copy link
Contributor

@jakubriegel jakubriegel commented Nov 25, 2023

Checklist

General

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.

Motivation and Context

Importing programming exercises from files created before merging of the continuous plagiarism control was not possible, because such files didn't contain plagiarism checks config. This caused the client to load the exercise creation form incorrectly.

Detected during tests of: #7624

Server part fixed in: #7640

Description

This PR adds a nullability check for exercise's plagiarism detection config before the exercise form is initialised. If exercise does not contain plagiarism detection config, the component inserts a default value into it.

Steps for Testing

Prerequisites:

  1. Go to course -> exercises.
  2. Start programming exercise from the file provided.
  3. Verify that the form displays correctly (including default plagiarism detection config).
  4. Import the exercise.
  5. Verify that the exercise was imported correctly.

Testserver States

Note

These badges show the state of the test servers.
Green = Currently available, Red = Currently locked







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
  • I (as a reviewer) 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

Class/File Line Coverage Confirmation (assert/expect)
exercise-update-plagiarism-component.ts 63% (testing just the new method)

@jakubriegel jakubriegel self-assigned this Nov 25, 2023
@github-actions github-actions bot added tests client Pull requests that update TypeScript code. (Added Automatically!) labels Nov 25, 2023
@jakubriegel jakubriegel marked this pull request as ready for review November 25, 2023 10:22
@jakubriegel jakubriegel requested a review from a team as a code owner November 25, 2023 10:22
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.

Code LGTM

@Strohgelaender
Copy link
Contributor

Thanks for fixing this, but I already implemented #7657 with the same fix.

Copy link

@JanaNF JanaNF left a comment

Choose a reason for hiding this comment

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

I tried to test this PR on TS2, Legcay_TS1, Legacy_TS2 and Legacy_TS11, and I always only saw the loading screen ("We'll be back soon"). I don't know what problems Artemis has right now but I had the same issue with other PRs too.

I will try to test this PR again tomorrow.

Copy link
Contributor

@sarpsahinalp sarpsahinalp 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. Worked as expected

Copy link

@JanaNF JanaNF left a comment

Choose a reason for hiding this comment

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

Manual tested on ts2. Worked as expected.

@jakubriegel
Copy link
Contributor Author

Thanks for fixing this, but I already implemented #7657 with the same fix.

This is continuation of work started in #7640

Copy link
Contributor

@Strohgelaender Strohgelaender 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, let's merge that quickly.

@krusche krusche modified the milestones: 6.7.1, 6.7.0 Nov 26, 2023
@krusche krusche merged commit e4a7d11 into develop Nov 26, 2023
27 of 31 checks passed
@krusche krusche deleted the plagiarism-checks-fix-import-exercise-with-no-plagiarism-detection-config branch November 26, 2023 22:11
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 tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants