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

Programming exercises: Delete directory for import from file if error occurs #6864

Merged

Conversation

tobias-lippert
Copy link
Contributor

@tobias-lippert tobias-lippert commented Jul 8, 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.

Changes affecting Programming Exercises

  • I tested all changes and their related features with all corresponding user types on Test Server 1 (Atlassian Suite).
  • I tested all changes and their related features with all corresponding user types on Test Server 2 (Jenkins and Gitlab).

Motivation and Context

Currently, when importing a programming exercise from file, the directory is only scheduled for the deletion if the import is successful.
We, however, want to delete it regardless if the import fails or not.

Description

Wrap the import process in a try block and the directory deletion scheduling in a finally block, so it's always executed.
Since this is hard to test on a test server without access to the file system, I added server tests, to ensure the deletion method is invoked.

Steps for Testing

Code review only.

Review Progress

Performance Review

  • 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

Test Coverage

Server

Class/File Line Coverage Confirmation (assert/expect)
ProgrammingExerciseImportFromFileService.java 95%

@tobias-lippert tobias-lippert requested a review from a team as a code owner July 8, 2023 10:13
@github-actions github-actions bot added tests server Pull requests that update Java code. (Added Automatically!) labels Jul 8, 2023
@tobias-lippert tobias-lippert changed the title Development: Delete directory for import from file if error occurs Programming exercises: Delete directory for import from file if error occurs Jul 8, 2023
@tobias-lippert tobias-lippert added this to the 6.3.3 milestone Jul 8, 2023
JohannesStoehr
JohannesStoehr previously approved these changes Jul 8, 2023
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 should fix the issue. Insurgent simply putting the deletion right after getting the folder instead of putting it into a finally block

@krusche krusche modified the milestones: 6.3.3, 6.3.4 Jul 8, 2023
terlan98
terlan98 previously approved these changes Jul 9, 2023
Copy link
Contributor

@terlan98 terlan98 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. There's a star import that you could replace, but it's non-blocking.

@tobias-lippert tobias-lippert dismissed stale reviews from terlan98 and JohannesStoehr via aa17cb6 July 9, 2023 12:15
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.

Only changed imports. Still look good to me

@krusche krusche merged commit c9fa6ea into develop Jul 13, 2023
@krusche krusche deleted the enhancement/import-from-file-delete-directory-if-exception branch July 13, 2023 10:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge server Pull requests that update Java code. (Added Automatically!) tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants