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

General: Add cleanup service for admins #9296

Open
wants to merge 109 commits into
base: develop
Choose a base branch
from

Conversation

coolchock
Copy link
Contributor

@coolchock coolchock commented Sep 7, 2024

Checklist

General

Server

  • Important: I implemented the changes with a very good performance and prevented too many (unnecessary) and too complex database calls.
  • I strictly followed the principle of data economy for all database calls.
  • I strictly followed the server coding and design guidelines.
  • I added multiple integration tests (Spring) related to the features (with a high test coverage).
  • I added pre-authorization annotations according to the guidelines and checked the course groups for all new REST Calls (security).
  • 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 (e.g. using paging).
  • I strictly followed the principle of data economy for all client-server REST calls.
  • I strictly followed the client coding and design guidelines.
  • 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 added multiple integration tests (Jest) related to the features (with a high test coverage), while following the test guidelines.
  • I added authorities to all new routes and checked the course groups for displaying navigation elements (links, buttons).
  • 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

Artemis stores a large amount of information, some of which can become corrupted or irrelevant over time. These records occupy significant database space, which can be reclaimed by deleting unused data. This includes old results, unresolved plagiarism cases, and unused feedback.

Currently, there are scripts for cleaning up results and feedback, but no automated process exists for clearing unresolved plagiarism cases. Additionally, orphaned entities, such as results without associated submissions or participation, remain unaddressed, leading to inefficient database usage. Furthermore, these cleanup scripts must be run manually by administrators, which is not ideal. To address these issues, a dedicated cleanup service for administrators was implemented to streamline the process.

Description

A cleanup service has been added to the admin panel, allowing administrators to select specific cleanup operations to perform on the database. They can specify a time period for which the cleanup should be executed, providing flexibility in managing old or unused data. In future iterations, a scheduled job is planned to be introduced, which will automatically execute the cleanup operations periodically, reducing the need for manual intervention and ensuring ongoing database optimization.

Steps for Testing

Prerequisites:

  • 1 Administrator
  • 1 Test Server where you have access rights
  • Test data generated using large_course_main.py script. See README in the folder quick-course-setup for more details.
  1. Log in to Artemis.
  2. Navigate to course management and select the course created by the script.
  3. Set the start and end dates of the course within the period for which you will delete data.
  4. Open the programming exercise created by the script and go to the Plagiarism section.
  5. Set the threshold to 0 and execute plagiarism detection. You should receive a few results.
  6. In Server Administration -> Cleanup Service, execute "Delete plagiarism comparisons."
  7. Verify that the plagiarism comparisons were deleted.
  8. Execute the "Delete old rated results" operation.
  9. Verify that only the last student submission has a linked result.
  10. Set the end date for the programming exercise to a past date and start a practice mode participation.
  11. In the cleanup service, execute "Delete non-rated results."
  12. Verify that the practice mode submission no longer has a linked result.

Testserver States

Note

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







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 even for very large courses with more than 2000 students.
  • I (as a reviewer) confirm that the server changes (in particular related to database calls) are implemented with a very good performance even for very large courses with more than 2000 students.

Code Review

  • Code Review 1
  • Code Review 2

Manual Tests

  • Test 1
  • Test 2

Performance Tests

  • Test 1
  • Test 2

Test Coverage

Info: ✅ ❌ in Confirmation (assert/expect) have to be adjusted manually, also delete trivial files!

Client

Class/File Line Coverage Confirmation (assert/expect)
admin.route.ts not found (modified)
cleanup-operation.model.ts 0% ✅ ❌
cleanup-service.component.ts 82.5% ✅ ❌
data-cleanup.service.ts.ts 100%
date-time-picker.component.ts 96.66% ✅ ❌
navbar.component.ts 87.76% ✅ ❌

Server

Class/File Line Coverage Confirmation (assert/expect)
FeedbackRepository.java 86% ✅ ❌
LongFeedbackTextRepository.java 100%
ParticipantScoreRepository.java 81% ✅ ❌
RatingRepository.java not found (modified)
ResultRepository.java 96% ✅ ❌
StudentScoreRepository.java not found (modified)
TeamScoreRepository.java not found (modified)
TextBlockRepository.java not found (modified)
CleanupJobExecution.java 80% ✅ ❌
CleanupJobType.java 100%
CleanupServiceExecutionRecordDTO.java 100%
CleanupJobExecutionRepository.java not found (added)
DataCleanupService.java 100%
AdminOldDataCleanupResource.java 100%
PlagiarismComparisonRepository.java 100%

Screenshots

View from the navigation bar
image

Cleanup service
image

Confirmation dialog
image

Summary by CodeRabbit

Release Notes

  • New Features

    • Introduced a comprehensive cleanup service for managing orphaned data and outdated results.
    • Added REST endpoints for administrators to execute cleanup operations, including deleting orphaned data and plagiarism comparisons.
    • Enhanced user interface with a new component for displaying and executing cleanup operations, featuring date validation and confirmation prompts.
    • New entries added to the admin menu for the cleanup service in both German ("Aufräumdienst") and English ("Cleanup Service").
  • Bug Fixes

    • Disabled certain test classes to streamline testing processes without altering existing logic.

@github-actions github-actions bot added tests server Pull requests that update Java code. (Added Automatically!) client Pull requests that update TypeScript code. (Added Automatically!) labels Sep 7, 2024
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 12

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE

📥 Commits

Files that changed from the base of the PR and between 31fc02e and cf41472.

📒 Files selected for processing (3)
  • src/main/java/de/tum/cit/aet/artemis/core/service/cleanup/DataCleanupService.java (1 hunks)
  • src/main/java/de/tum/cit/aet/artemis/core/web/admin/AdminOldDataCleanupResource.java (1 hunks)
  • src/main/webapp/app/admin/cleanup-service/cleanup-service.component.html (1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
src/main/java/de/tum/cit/aet/artemis/core/service/cleanup/DataCleanupService.java (1)

Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports

src/main/java/de/tum/cit/aet/artemis/core/web/admin/AdminOldDataCleanupResource.java (1)

Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports

src/main/webapp/app/admin/cleanup-service/cleanup-service.component.html (1)

Pattern src/main/webapp/**/*.html: @if and @for are new and valid Angular syntax replacing *ngIf and *ngFor. They should always be used over the old style.

📓 Learnings (1)
src/main/webapp/app/admin/cleanup-service/cleanup-service.component.html (1)
Learnt from: undernagruzez
PR: ls1intum/Artemis#9296
File: src/main/webapp/app/admin/cleanup-service/cleanup-service.component.ts:19-44
Timestamp: 2024-09-13T11:36:07.446Z
Learning: In `cleanup-service.component.ts`, the date range for cleanup operations is configurable through user input via the date time picker.
🔇 Additional comments (1)
src/main/java/de/tum/cit/aet/artemis/core/web/admin/AdminOldDataCleanupResource.java (1)

100-100: ⚠️ Potential issue

Ensure method names reflect called service methods for consistency

The controller method deleteOldRatedResults is calling dataCleanupService.deleteNonLatestRatedResults, which may cause confusion due to differing method names.

Please confirm if the intended service method is deleteOldRatedResults. If so, consider renaming the service method for consistency.

Apply this diff to rename the service method call:

-    CleanupServiceExecutionRecordDTO result = dataCleanupService.deleteNonLatestRatedResults(deleteFrom, deleteTo);
+    CleanupServiceExecutionRecordDTO result = dataCleanupService.deleteOldRatedResults(deleteFrom, deleteTo);

Additionally, ensure that the dataCleanupService has a method named deleteOldRatedResults.

coderabbitai[bot]
coderabbitai bot previously approved these changes Oct 4, 2024
Copy link
Contributor

@SimonEntholzer SimonEntholzer left a comment

Choose a reason for hiding this comment

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

I think there is an issue with the plagiarism deletion, or I'm doing something wrong.
I've got an exercise with multiple plagiarism comparisons on Ts3: https://artemis-test3.artemis.cit.tum.de/course-management/51/text-exercises/569/plagiarism
But for some reason the clean up service does not delete them, when I select the time-frame in which I created them.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE

📥 Commits

Files that changed from the base of the PR and between cf41472 and 1afaff4.

📒 Files selected for processing (1)
  • src/main/webapp/app/admin/cleanup-service/cleanup-service.component.html (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/main/webapp/app/admin/cleanup-service/cleanup-service.component.html (1)

Pattern src/main/webapp/**/*.html: @if and @for are new and valid Angular syntax replacing *ngIf and *ngFor. They should always be used over the old style.

📓 Learnings (1)
src/main/webapp/app/admin/cleanup-service/cleanup-service.component.html (1)
Learnt from: undernagruzez
PR: ls1intum/Artemis#9296
File: src/main/webapp/app/admin/cleanup-service/cleanup-service.component.ts:19-44
Timestamp: 2024-09-13T11:36:07.446Z
Learning: In `cleanup-service.component.ts`, the date range for cleanup operations is configurable through user input via the date time picker.
🔇 Additional comments (1)
src/main/webapp/app/admin/cleanup-service/cleanup-service.component.html (1)

17-17: Verify the usage of the track option in the @for directive

Please ensure that using track operation in the @for directive is correct. In Angular, the tracking mechanism typically uses trackBy along with a unique identifier or function to optimize rendering performance.

Run the following script to check for other usages of @for with track in the codebase:

This script will help confirm if the usage of track operation aligns with the established patterns in the project.

✅ Verification successful

Verified: The usage of track operation in the @for directive is consistent with project standards and correctly implemented.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Search for `@for` directives using 'track' in HTML files.

rg --glob '*.html' '@for.*;[[:space:]]*track' 

Length of output: 77059

@undernagruzez undernagruzez added deploy:artemis-test3 and removed deployment-error Added by deployment workflows if an error occured labels Oct 5, 2024
@github-actions github-actions bot added deployment-error Added by deployment workflows if an error occured and removed deploy:artemis-test3 labels Oct 5, 2024
@krusche krusche modified the milestones: 7.5.6, 7.6.0 Oct 5, 2024
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Outside diff range comments (1)
src/main/webapp/i18n/en/global.json (1)

Line range hint 101-120: Consider standardizing punctuation in the "admin" section.

While the new entry follows the style of some existing entries, there's an inconsistency in the use of periods at the end of values in the "admin" section. For improved maintainability and consistency, consider standardizing the punctuation across all entries in a future update.

Example of inconsistency:

"userManagement": "User management",
"organizationManagement": "Organization management",
"systemNotifications": "System notifications",
"featureToggles": "Feature Toggles",
"statistics": "User statistics",
"metrics": "Metrics",
"health": "Health",
"configuration": "Configuration",
"logs": "Logs",
"audits": "Audits",
"apidocs": "API",
"privacyStatement": "Privacy Statement",
"imprint": "Imprint",
"database": "Database",
"iris": "Iris Settings",
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE

📥 Commits

Files that changed from the base of the PR and between 1afaff4 and 1339ca2.

📒 Files selected for processing (2)
  • src/main/webapp/i18n/de/global.json (1 hunks)
  • src/main/webapp/i18n/en/global.json (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/main/webapp/i18n/de/global.json (1)

Pattern src/main/webapp/i18n/de/**/*.json: German language translations should be informal (dutzen) and should never be formal (sietzen). So the user should always be addressed with "du/dein" and never with "sie/ihr".

🔇 Additional comments (1)
src/main/webapp/i18n/de/global.json (1)

118-119: LGTM! The translation looks good and addresses previous comments.

The new entry for "cleanupService" with the translation "Aufräumdienst" is appropriate and uses informal German as required. This change successfully addresses the previous review comments about translating "Cleanup Service" to German.

src/main/webapp/i18n/en/global.json Show resolved Hide resolved
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

Copy link
Contributor

@pzdr7 pzdr7 left a comment

Choose a reason for hiding this comment

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

Code

Copy link
Contributor

@edkaya edkaya 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 modified the milestones: 7.6.0, 7.6.1 Oct 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assessment Pull requests that affect the corresponding module client Pull requests that update TypeScript code. (Added Automatically!) core Pull requests that affect the corresponding module database Pull requests that update the database. (Added Automatically!). Require a CRITICAL deployment. deployment-error Added by deployment workflows if an error occured plagiarism Pull requests that affect the corresponding module server Pull requests that update Java code. (Added Automatically!) tests
Projects
Status: Developer Approved
Development

Successfully merging this pull request may close these issues.

8 participants