Improve data privacy handling and per-group settings for multi-group usage#864
Merged
Improve data privacy handling and per-group settings for multi-group usage#864
Conversation
Completely rewrites the privacy statement to accurately reflect the application's data processing activities. Adds sections for user account data, uploaded documents, thesis data, email notifications, calendar feeds, authentication, server logging, data retention periods, data recipients, and full GDPR rights. Also adds a data retention policy document with rationale for retention periods. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The MAIL_BCC_RECIPIENTS environment variable was read but never used. The actual BCC functionality uses the research group head directly. Removes the dead config from MailConfig, application.yml, Docker Compose, GitHub Actions workflow, and documentation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Remove automatic Gravatar URL generation from User entity and client. Add opt-in "Import from Gravatar" button that fetches the profile picture server-side using SHA-256 email hashing, so the user's IP is never exposed to the external service. Add one-time migration task to download existing profile pictures for users without a custom avatar. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…atic application expiration Mention interview assessment notes in the privacy statement. Add automatic application expiration documentation to README (user-facing) and DATA_RETENTION.md (internal rationale with GDPR Art. 22 clarification). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace flat checklist with prioritized sections. Add configurable application email content as highest priority item based on feedback from Lehrstuhl für Mikro- und Nanosystemtechnik. Include CalDAV removal and migration cleanup as low-priority items. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…tion Add server-side consent tracking, privacy statement versioning, account/data deletion endpoint, and data export endpoint to the prioritized TODO list. Items from a previous review that are already resolved (Gravatar, notification preferences, log retention, email logging) are excluded. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Merge consent tracking and privacy statement versioning into a single item. Document the current localStorage workaround (UX convenience for pre-ticking the checkbox) and describe the target implementation with User entity fields and version-based re-prompting. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Move data export and automatic deletion of rejected applications to highest priority as they address concrete GDPR rights (Art. 20, Art. 17) that users can exercise today. Move configurable email content to medium priority. Move consent tracking to low priority since there is no versioned privacy statement yet and implicit consent through application submission is sufficient for now. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The Lehrstuhl für Mikro- und Nanosystemtechnik has already raised this concern. Responding promptly demonstrates good faith; ignoring an active complaint risks escalation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Data export requests are currently handled manually, so there is no immediate compliance gap. A self-service feature remains desirable to reduce administrative effort. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… global variables
- Move DataRetentionCleanup to DataRetentionService (@service, public API returning count) - Add DELETE /v2/applications/{id} endpoint (admin-only) for individual application deletion - Add POST /v2/data-retention/cleanup-rejected-applications endpoint for on-demand batch cleanup - Add ApplicationDeleteButton component (admin-only, with confirmation modal) - Add AdminPage with data retention panel and "Run Cleanup" button - Add /admin route and Administration nav link for admins - Add old rejected applications to seed data for E2E testing - Add E2E tests for individual delete, batch cleanup, and non-admin restrictions Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Fix path traversal vulnerability in data export download by validating file path stays within export directory - Fix canRequest boolean omitted from JSON when false (primitive boolean considered "empty" by @JsonInclude(NON_EMPTY)), changed to Boolean - Fix HttpClient and InputStream resource leaks in Gravatar import by using try-with-resources - Fix orphaned ZIP files on disk when export creation fails mid-way by cleaning up partial files in extracted writeZipFile method - Fix Modal nested inside Button in ApplicationDeleteButton (invalid HTML) - Extract duplicate sha256Hex and Gravatar logic into shared GravatarService Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…tures - Add data export, profile picture import, and data retention to README - Add scientific writing guide and admin features to README - Add Data Retention Policy to developer documentation links - Add DATA_EXPORT_PATH, DATA_EXPORT_RETENTION_DAYS, DATA_EXPORT_COOLDOWN_DAYS environment variables to CONFIGURATION.md - Mark data export TODO as completed in DATA_RETENTION.md - Add data-exports volume and backup note to PRODUCTION.md Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add a nightly job that identifies student accounts with no activity (login, profile update, application, or data export) for over a year and disables them. Users with active theses or non-student roles are excluded. Disabled accounts are automatically re-enabled on login. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add self-service and admin account deletion with two scenarios: - Full deletion when no retention-blocked data exists - Soft-deletion (deactivate + schedule) when thesis data is under 5-year legal retention, preserving profile and thesis-related files so professors can still find theses by student name Includes: Liquibase migration for deletion tracking columns and FK constraint changes, UserDeletionService with preview/delete logic, REST endpoints, auth guard for deleted accounts, nightly deferred deletion job, Settings page Account tab, and Admin page deletion UI. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Delete ApplicationReviewers before their parent Applications to prevent Hibernate session conflicts during user deletion - Format retention dates as human-readable (e.g. "December 31, 2030") instead of ISO timestamps - Update test to include ApplicationReviewer in rejected application setup Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…nsactional - Add server-side guard preventing deletion of accepted applications (FK to theses) - Disable delete button for accepted apps with explanatory tooltip - Update ReviewApplicationPage to clear selection after deletion - Remove @transactional from deleteApplication and use entity-based deletion with explicit ApplicationReviewer cleanup to avoid Hibernate session issues Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Require users to type their full name to confirm account deletion - Delete button stays disabled until the typed name matches exactly - Fix logout after deletion: navigate to /logout route instead of calling auth.logout() directly, which raced with AuthenticatedArea's auto-login effect that re-triggered keycloak.login() when tokens were cleared - Update E2E tests to fill in the confirmation name input Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Privacy-first: by default, application notification emails to supervisors and examiners only contain student name, topic, and link. When the setting is enabled, emails include full personal details and file attachments. Student confirmation emails are unaffected. Includes server/e2e tests and adds server/data-exports/ to .gitignore. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
E2E tests fail on second+ runs because seed data uses ON CONFLICT DO NOTHING, so state modified by prior runs is never reset. Adding docker compose down -v ensures PostgreSQL starts empty each time. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe pull request adds comprehensive GDPR/data-retention features (data export, soft/full user deletion with retention windows, admin controls), implements server-side Gravatar import and profile-picture migration, removes CalDAV/calendar integration, and updates database schema, APIs, frontend UI, and e2e/integration tests accordingly. Changes
Sequence Diagram(s)sequenceDiagram
actor Student
participant Client as Client App
participant Server as Backend
participant DB as Database
participant FS as File System
participant Email as Email Service
Student->>Client: Request Data Export
Client->>Server: POST /v2/data-exports
alt Can Request
Server->>DB: Create DataExport (REQUESTED)
DB-->>Server: Export ID
Server->>Client: 200 OK with status
else Rate Limited
Server->>Client: 429 Too Many Requests
end
Client->>Server: GET /v2/data-exports/status
Server->>DB: Query latest export
DB-->>Server: Export record
Server->>Client: Status with state, dates
alt Export Ready
Student->>Client: Download Export
Client->>Server: GET /v2/data-exports/{id}/download
Server->>DB: Validate ownership, check state
Server->>FS: Read ZIP file
FS-->>Server: File bytes
Server->>DB: Mark as DOWNLOADED
Server->>Client: File stream
Client-->>Student: Save ZIP
end
par Background Processing
Server->>DB: Find REQUESTED exports
Server->>DB: Claim for processing (IN_CREATION)
Server->>Server: Build user data JSON
Server->>Server: Build applications JSON
Server->>Server: Build theses JSON
Server->>FS: Create ZIP
Server->>FS: Add user files
FS-->>Server: ZIP path
Server->>DB: Update export (EMAIL_SENT/FAILED)
Server->>Email: Send ready notification
Email-->>Student: Export ready email
end
sequenceDiagram
actor Admin
participant AdminUI as Admin Page
participant Server as Backend
participant DB as Database
participant Email as Email
Admin->>AdminUI: Search for user
AdminUI->>Server: GET /v2/user-deletion/{userId}/preview
Server->>DB: Fetch user, theses, roles
DB-->>Server: User state, active theses, retention blocks
Server->>Server: Compute retention expiry
Server->>AdminUI: UserDeletionPreviewDto
AdminUI-->>Admin: Show preview (can delete?, active theses, retention)
alt User has active theses or is group head
AdminUI-->>Admin: Delete button disabled
else User eligible
Admin->>AdminUI: Confirm deletion
AdminUI->>Server: DELETE /v2/user-deletion/{userId}
Server->>DB: Start transaction
Server->>DB: Delete applications, files
alt User has retention-blocked data
Server->>DB: Mark disabled, set deletion_scheduled_for
Server->>DB: Clear sensitive fields
else No retention blocks
Server->>DB: Anonymize user record
Server->>DB: Delete remaining applications
Server->>DB: Delete roles, files
end
Server->>DB: Commit
Server->>AdminUI: UserDeletionResultDto
AdminUI-->>Admin: Deletion complete
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
client/src/utils/user.ts (1)
1-2:⚠️ Potential issue | 🟡 MinorUse
import typefor type-only imports and add optional chaining with URL encoding.Line 2:
IMinimalUseris a type—change toimport type { IMinimalUser } from '../requests/responses/user'.Lines 5–6: The
getAvatarfunction should handle potentially missing DTO fields and safely encode URL parameters:
- Use optional chaining for
user.avataranduser.userId- URL-encode
user.avatarto prevent malformed URLs when it contains special charactersimport { GLOBAL_CONFIG } from '../config/global' import type { IMinimalUser } from '../requests/responses/user' export function getAvatar(user: IMinimalUser) { return user.avatar ? `${GLOBAL_CONFIG.server_host}/api/v2/avatars/${user.userId ?? ''}?filename=${encodeURIComponent(user.avatar ?? '')}` : undefined }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/utils/user.ts` around lines 1 - 2, Change the type import to a type-only import (import type { IMinimalUser } from '../requests/responses/user') and update getAvatar to guard against missing fields by using optional chaining for user.avatar and user.userId, URL-encoding the avatar filename with encodeURIComponent, and falling back to an empty string for userId when building the URL that uses GLOBAL_CONFIG.server_host and the /api/v2/avatars/{userId} endpoint.server/src/main/java/de/tum/cit/aet/thesis/dto/ResearchGroupSettingsDTO.java (1)
1-12:⚠️ Potential issue | 🟠 MajorAdd
@JsonInclude(NON_EMPTY)to this DTO.This DTO lacks the required JsonInclude annotation.
Suggested change
package de.tum.cit.aet.thesis.dto; +import com.fasterxml.jackson.annotation.JsonInclude; import de.tum.cit.aet.thesis.entity.ResearchGroupSettings; +@JsonInclude(JsonInclude.Include.NON_EMPTY) public record ResearchGroupSettingsDTO(As per coding guidelines:
server/src/**/dto/**/*.java: All DTOs must use@JsonInclude(JsonInclude.Include.NON_EMPTY)annotation; null, empty strings, and empty collections are omitted from JSON serialization.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/src/main/java/de/tum/cit/aet/thesis/dto/ResearchGroupSettingsDTO.java` around lines 1 - 12, Add the Jackson annotation to the DTO so empty/null fields are omitted: annotate the record ResearchGroupSettingsDTO with `@JsonInclude`(JsonInclude.Include.NON_EMPTY) and import com.fasterxml.jackson.annotation.JsonInclude; ensure the import and annotation sit above the public record declaration for ResearchGroupSettingsDTO so nulls, empty strings, and empty collections are excluded during JSON serialization.server/src/test/java/de/tum/cit/aet/thesis/controller/ResearchGroupSettingsControllerTest.java (1)
14-14:⚠️ Potential issue | 🔴 CriticalChange import to
com.fasterxml.jackson.databind.JsonNode(Jackson 2.x).Project uses Spring Boot 4.0.3, which provides Jackson 2.x (
com.fasterxml.jackson). The importtools.jackson.databind(Jackson 3.x) is incorrect and will cause compilation errors. Update all test files usingtools.jacksonimports to usecom.fasterxml.jacksoninstead.Additionally, fix the anomalous import in
server/src/main/java/de/tum/cit/aet/thesis/exception/ResponseExceptionHandler.javawhich importstools.jackson.core.JacksonException.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/src/test/java/de/tum/cit/aet/thesis/controller/ResearchGroupSettingsControllerTest.java` at line 14, Replace incorrect Jackson 3.x imports with Jackson 2.x: change the import of JsonNode from tools.jackson.databind.JsonNode to com.fasterxml.jackson.databind.JsonNode in the test class (look for the JsonNode import line) and also change tools.jackson.core.JacksonException to com.fasterxml.jackson.core.JacksonException in the ResponseExceptionHandler class (look for JacksonException import); update any other test files with similar tools.jackson.* imports to com.fasterxml.jackson.* to match Spring Boot 4's Jackson 2.x API so the code compiles.
🟠 Major comments (18)
server/src/main/java/de/tum/cit/aet/thesis/service/UploadService.java-132-148 (1)
132-148:⚠️ Potential issue | 🟠 MajorValidate/sanitize
extensionto prevent path traversal instoreBytes.
extensionis concatenated into the filename without any clean-path or..guard, so a crafted value could escape the upload directory.🔒 Suggested fix
public String storeBytes(byte[] bytes, String extension, int maxSize) { try { if (bytes == null || bytes.length == 0) { throw new UploadException("Failed to store empty file"); } @@ MessageDigest digest = MessageDigest.getInstance("SHA-256"); byte[] hashBytes = digest.digest(bytes); String hash = HexFormat.of().formatHex(hashBytes); - String filename = hash + "." + extension; + if (!StringUtils.hasText(extension) + || extension.contains("..") + || extension.contains("/") + || extension.contains("\\")) { + throw new UploadException("File extension not allowed"); + } + String filename = StringUtils.cleanPath(hash + "." + extension.toLowerCase()); + if (filename.contains("..")) { + throw new UploadException("Cannot store file with relative path outside current directory"); + } Files.write(rootLocation.resolve(filename), bytes); return filename;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/src/main/java/de/tum/cit/aet/thesis/service/UploadService.java` around lines 132 - 148, The storeBytes method currently concatenates the provided extension into the filename which allows path traversal; sanitize or validate the extension before building filename in StoreService.storeBytes: reject or normalize any extension containing path-separators, dots, or unexpected characters and only allow a safe whitelist (e.g., alphanumeric, max length) or map to known allowed extensions; then construct the filename using the computed hash + "." + the validated extension and write to rootLocation.resolve(filename) so no crafted extension can escape the upload directory.server/src/main/java/de/tum/cit/aet/thesis/cron/ProfilePictureMigration.java-70-71 (1)
70-71:⚠️ Potential issue | 🟠 MajorAvoid logging user identifiers (PII) at INFO level.
user.getUniversityId()is a personal identifier (the user's TUM username). Logging it atINFOlevel means it lands in persistent production logs, creating an unnecessary PII retention risk.Drop
getUniversityId()from the log — the UUID is sufficient for correlation.🛡️ Proposed fix
- log.info("Profile picture migration: downloaded avatar for user {} ({})", - user.getUniversityId(), user.getId()); + log.info("Profile picture migration: downloaded avatar for user {}", user.getId());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/src/main/java/de/tum/cit/aet/thesis/cron/ProfilePictureMigration.java` around lines 70 - 71, The log entry in ProfilePictureMigration currently logs a personal identifier via user.getUniversityId(); remove that call from the log message and only log the UUID (user.getId()) to avoid persisting PII at INFO level — update the log.info invocation in ProfilePictureMigration to exclude user.getUniversityId() and ensure the message and formatting only reference user.getId().server/src/main/java/de/tum/cit/aet/thesis/dto/UserDeletionResultDto.java-1-6 (1)
1-6:⚠️ Potential issue | 🟠 MajorAdd
@JsonInclude(NON_EMPTY)annotation to this DTO.All DTOs in this module require the annotation to omit null, empty strings, and empty collections from JSON serialization.
Suggested fix
+import com.fasterxml.jackson.annotation.JsonInclude; + +@JsonInclude(JsonInclude.Include.NON_EMPTY) public record UserDeletionResultDto( String result, String message ) { }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/src/main/java/de/tum/cit/aet/thesis/dto/UserDeletionResultDto.java` around lines 1 - 6, Add the Jackson annotation to the UserDeletionResultDto record so empty/null values are omitted in JSON: annotate the record UserDeletionResultDto with `@JsonInclude`(JsonInclude.Include.NON_EMPTY) and add the necessary import for com.fasterxml.jackson.annotation.JsonInclude; place the annotation immediately above the record declaration to match other DTOs in the module.server/src/main/java/de/tum/cit/aet/thesis/service/ApplicationService.java-174-174 (1)
174-174: 🛠️ Refactor suggestion | 🟠 MajorMultiple TODO comments left in production code.
Eleven identical TODO comments were added throughout the file. Per coding guidelines, TODO comments should not remain in production code. If there's a genuine concern about
@Transactionalusage, track it as an issue/ticket rather than scattering TODOs across the codebase.As per coding guidelines: "Avoid leaving TODO comments in production code."
Also applies to: 208-208, 223-223, 277-277, 322-322, 393-393, 412-412, 429-429, 447-447, 455-455, 489-489
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/src/main/java/de/tum/cit/aet/thesis/service/ApplicationService.java` at line 174, Remove the eleven TODO comments about avoiding `@Transactional` from the ApplicationService class; instead open a tracked issue/ticket and replace each TODO with a short, non-actionable comment referencing that ticket ID (or remove the comment entirely) so production code contains no TODOs. Locate the comments in ApplicationService around the methods annotated with `@Transactional` and remove or replace them with a one-line reference to the created issue (e.g., "See ISSUE-XXXX for discussion on `@Transactional` usage") to preserve traceability without leaving TODOs in production.server/src/main/java/de/tum/cit/aet/thesis/dto/ResearchGroupSettingsApplicationEmailDTO.java-5-11 (1)
5-11: 🛠️ Refactor suggestion | 🟠 MajorMissing
@JsonInclude(JsonInclude.Include.NON_EMPTY)annotation.Per coding guidelines and project convention, all DTOs must include this annotation.
Fix
package de.tum.cit.aet.thesis.dto; +import com.fasterxml.jackson.annotation.JsonInclude; import de.tum.cit.aet.thesis.entity.ResearchGroupSettings; +@JsonInclude(JsonInclude.Include.NON_EMPTY) public record ResearchGroupSettingsApplicationEmailDTO( boolean includeApplicationDataInEmail ) {As per coding guidelines: "All DTOs must use
@JsonInclude(JsonInclude.Include.NON_EMPTY)annotation; null, empty strings, and empty collections are omitted from JSON serialization." Based on learnings from@kruschein PR#820: "Apply JsonInclude(JsonInclude.Include.NON_EMPTY) to all DTOs to minimize payload size."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/src/main/java/de/tum/cit/aet/thesis/dto/ResearchGroupSettingsApplicationEmailDTO.java` around lines 5 - 11, Add the missing Jackson annotation to the DTO: annotate the ResearchGroupSettingsApplicationEmailDTO record with `@JsonInclude`(JsonInclude.Include.NON_EMPTY) so that JSON serialization omits nulls, empty strings and empty collections; update imports accordingly and ensure the annotation is applied on the record declaration (ResearchGroupSettingsApplicationEmailDTO) to follow project DTO conventions.server/src/main/java/de/tum/cit/aet/thesis/service/ApplicationService.java-521-532 (1)
521-532:⚠️ Potential issue | 🟠 MajorRemove the explicit reviewer cleanup and rely on the CASCADE FK from migration 27.
The
deleteApplicationmethod explicitly deletes reviewers before deleting the application (lines 529–530), then deletes the application itself (line 531). Since the foreign key constraint added in migration 27 specifiesON DELETE CASCADE, these manual operations are redundant and create an unnecessary data consistency risk: if the application delete fails, the reviewers are already gone.Simplify to:
Suggested fix
public void deleteApplication(UUID applicationId) { Application application = findById(applicationId); if (application.getState() == ApplicationState.ACCEPTED) { throw new ResourceInvalidParametersException( "Accepted applications cannot be deleted because they are linked to a thesis."); } - applicationReviewerRepository.deleteAll(application.getReviewers()); - application.getReviewers().clear(); applicationRepository.delete(application); }The cascade will handle reviewer deletion automatically and atomically at the database level.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/src/main/java/de/tum/cit/aet/thesis/service/ApplicationService.java` around lines 521 - 532, The deleteApplication method in ApplicationService currently manually deletes reviewers via applicationReviewerRepository.deleteAll(application.getReviewers()) and application.getReviewers().clear() before calling applicationRepository.delete(application); remove those two explicit reviewer cleanup calls and rely on the DB-level ON DELETE CASCADE from migration 27 so that deleting the Application (in deleteApplication(UUID applicationId)) will atomically remove related reviewers; keep the existing accepted-state check and the final applicationRepository.delete(application) call.server/src/main/resources/db/changelog/changes/28_data_export.sql-4-12 (1)
4-12: 🛠️ Refactor suggestion | 🟠 MajorMissing index on
user_idforeign key.Queries for a user's export status and nightly cleanup jobs will scan the full table without an index on
user_id.✅ Proposed fix
CREATE TABLE data_exports ( data_export_id UUID PRIMARY KEY DEFAULT gen_random_uuid(), user_id UUID NOT NULL REFERENCES users(user_id), state TEXT NOT NULL DEFAULT 'REQUESTED', file_path TEXT, created_at TIMESTAMP NOT NULL DEFAULT NOW(), creation_finished_at TIMESTAMP, downloaded_at TIMESTAMP ); + +CREATE INDEX idx_data_exports_user_id ON data_exports(user_id);As per coding guidelines: "Add proper indexes for foreign keys and frequently queried columns."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/src/main/resources/db/changelog/changes/28_data_export.sql` around lines 4 - 12, Add an index on the data_exports.user_id foreign key to avoid full-table scans when querying a user's exports or running cleanup jobs: update the migration that creates the data_exports table (table name: data_exports, column: user_id) to include a CREATE INDEX for user_id; optionally also add a composite index on (user_id, state) if your queries/filtering frequently include the state column (state) to speed status lookups and cleanup.server/src/main/java/de/tum/cit/aet/thesis/dto/ResearchGroupSettingsWritingGuideDTO.java-1-11 (1)
1-11: 🛠️ Refactor suggestion | 🟠 MajorMissing
@JsonInclude(JsonInclude.Include.NON_EMPTY)annotation.Without it, a null
scientificWritingGuideLinkwill be serialized as"scientificWritingGuideLink": nullinstead of being omitted from the payload.✅ Proposed fix
package de.tum.cit.aet.thesis.dto; +import com.fasterxml.jackson.annotation.JsonInclude; import de.tum.cit.aet.thesis.entity.ResearchGroupSettings; +@JsonInclude(JsonInclude.Include.NON_EMPTY) public record ResearchGroupSettingsWritingGuideDTO( String scientificWritingGuideLink ) {As per coding guidelines: "All DTOs must use
@JsonInclude(JsonInclude.Include.NON_EMPTY)annotation." Also based on learnings from PR#820: "ApplyJsonInclude(JsonInclude.Include.NON_EMPTY)to all DTOs to minimize payload size."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/src/main/java/de/tum/cit/aet/thesis/dto/ResearchGroupSettingsWritingGuideDTO.java` around lines 1 - 11, Add the Jackson non-empty inclusion annotation to the DTO so null/empty scientificWritingGuideLink is omitted: import com.fasterxml.jackson.annotation.JsonInclude and annotate the record ResearchGroupSettingsWritingGuideDTO with `@JsonInclude`(JsonInclude.Include.NON_EMPTY); leave the existing record components and the fromEntity(ResearchGroupSettings settings) method unchanged so serialization behavior is adjusted without altering construction logic.server/src/test/java/de/tum/cit/aet/thesis/service/MailingServiceIntegrationTest.java-140-182 (1)
140-182:⚠️ Potential issue | 🟠 MajorThe test's core assertion is missing — test name is misleading.
createApplication_WithSettingEnabled_ChairEmailHasAttachmentsnever asserts that the chair email actually has attachments. The only assertions are that at least one email was sent and the student received one, which are identical to the baseline test. ThecountAttachmentshelper is never called here.✅ Proposed fix — add the chair attachment assertion
assertThat(allRecipients).as("Student should receive an email") .anyMatch(addr -> addr.contains(student.universityId())); + + // Chair email should include attachments when setting is enabled + boolean chairEmailVerified = false; + for (MimeMessage email : emails) { + List<String> recipients = Arrays.stream(email.getAllRecipients()) + .map(Address::toString) + .toList(); + if (recipients.stream().anyMatch(addr -> addr.contains(head.universityId()))) { + assertThat(countAttachments(email)) + .as("Chair email should have attachments when includeApplicationDataInEmail is enabled") + .isGreaterThan(0); + chairEmailVerified = true; + } + } + assertThat(chairEmailVerified).as("At least one chair email should have been found and verified").isTrue(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/src/test/java/de/tum/cit/aet/thesis/service/MailingServiceIntegrationTest.java` around lines 140 - 182, The test createApplication_WithSettingEnabled_ChairEmailHasAttachments doesn't assert that the chair's email contains attachments; update the test to locate the MimeMessage for the chair (use head.universityId() and getReceivedEmails()/getAllRecipientAddresses() to find the message) and call the existing countAttachments helper on that MimeMessage, then assert the attachment count is greater than zero (e.g., assertTrue or assertThat(countAttachments(chairMessage)).isGreaterThan(0)). Ensure you reference the createApplication_WithSettingEnabled_ChairEmailHasAttachments test, the head variable, getReceivedEmails(), getAllRecipientAddresses(), and countAttachments() when making the change.server/src/main/java/de/tum/cit/aet/thesis/service/MailingService.java-77-98 (1)
77-98:⚠️ Potential issue | 🟠 MajorThe
includeDatatoggle only controls attachment inclusion, not email body content.Both
prepareApplicationCreatedMailBuilderandprepareMinimalApplicationMailBuildercall the identicalfillApplicationPlaceholders(application)method, which injects the completeMailApplicationrecord into the template. This exposes sensitive data (motivation, special skills, interests, email, university ID, matriculation number, study program/degree) in the email body regardless of the toggle setting. Only the attachments (CV, examination report, degree report) are conditionally included.Consider using a dedicated minimal template without application placeholders, or create a separate placeholder-filling method that only injects essential fields when
includeDatais false.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/src/main/java/de/tum/cit/aet/thesis/service/MailingService.java` around lines 77 - 98, The includeData toggle only controls attachments but not the email body: both prepareApplicationCreatedMailBuilder and prepareMinimalApplicationMailBuilder call fillApplicationPlaceholders(application) and therefore leak sensitive fields; change the logic so the minimal path does not inject full application data—either load a dedicated minimal EmailTemplate (e.g., "APPLICATION_CREATED_CHAIR_MINIMAL") in sendApplicationCreatedEmail or implement a new filler (e.g., fillMinimalApplicationPlaceholders(Application)) and call it from prepareMinimalApplicationMailBuilder; ensure sendNotificationCopy also uses the minimal builder/template when includeData is false so both the main send and notification copy omit sensitive placeholders.client/src/pages/ResearchGroupSettingPage/components/ApplicationEmailContentSettingsCard.tsx-32-40 (1)
32-40:⚠️ Potential issue | 🟠 MajorGuard against missing
applicationEmailSettingsin the response.If the server omits
applicationEmailSettings(e.g., older records or NON_EMPTY DTOs), this access can throw. Please use optional chaining + a default.Suggested change
- if ( - res.data.applicationEmailSettings.includeApplicationDataInEmail !== - includeApplicationDataInEmail - ) { - setIncludeApplicationDataInEmail( - res.data.applicationEmailSettings.includeApplicationDataInEmail, - ) - } + const serverValue = + res.data?.applicationEmailSettings?.includeApplicationDataInEmail ?? false + if (serverValue !== includeApplicationDataInEmail) { + setIncludeApplicationDataInEmail(serverValue) + }As per coding guidelines: Client code must handle missing DTO fields with
?? ''for strings,?? []for arrays, and?.for optional chaining.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/pages/ResearchGroupSettingPage/components/ApplicationEmailContentSettingsCard.tsx` around lines 32 - 40, Guard against missing applicationEmailSettings in the response by using optional chaining and a sensible default when reading res.data.applicationEmailSettings; update the comparison and setter inside ApplicationEmailContentSettingsCard (the block that references includeApplicationDataInEmail and calls setIncludeApplicationDataInEmail) to read res.data.applicationEmailSettings?.includeApplicationDataInEmail with a fallback (e.g., ?? false) so the code won’t throw when applicationEmailSettings is absent.client/src/pages/ResearchGroupSettingPage/components/ApplicationEmailContentSettingsCard.tsx-20-72 (1)
20-72:⚠️ Potential issue | 🟠 MajorAvoid leaving the toggle in a wrong state when the update fails (and guard missing route param).
Right now the UI stays toggled even if the request fails, which can mislead users about the actual server setting. Also guard against an undefined
researchGroupIdto avoid/undefinedrequests.Suggested change
- const handleChange = (value: boolean) => { - doRequest<IResearchGroupSettings>( + const handleChange = (value: boolean) => { + if (!researchGroupId) { + showSimpleError('Research group not found.') + return + } + const previousValue = includeApplicationDataInEmail + setIncludeApplicationDataInEmail(value) + doRequest<IResearchGroupSettings>( `/v2/research-group-settings/${researchGroupId}`, { method: 'POST', requiresAuth: true, data: { applicationEmailSettings: { includeApplicationDataInEmail: value, }, }, }, (res) => { if (res.ok) { - if ( - res.data.applicationEmailSettings.includeApplicationDataInEmail !== - includeApplicationDataInEmail - ) { - setIncludeApplicationDataInEmail( - res.data.applicationEmailSettings.includeApplicationDataInEmail, - ) - } + const serverValue = + res.data?.applicationEmailSettings?.includeApplicationDataInEmail ?? false + setIncludeApplicationDataInEmail(serverValue) } else { + setIncludeApplicationDataInEmail(previousValue) showSimpleError(getApiResponseErrorMessage(res)) } }, ) } @@ - <Switch - checked={includeApplicationDataInEmail} - onChange={(event) => { - setIncludeApplicationDataInEmail(event.currentTarget.checked) - handleChange(event.currentTarget.checked) - }} - /> + <Switch + checked={includeApplicationDataInEmail} + onChange={(event) => handleChange(event.currentTarget.checked)} + />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/pages/ResearchGroupSettingPage/components/ApplicationEmailContentSettingsCard.tsx` around lines 20 - 72, The toggle is left in the wrong state when the API update fails and we also send requests to /undefined if researchGroupId is missing; fix by guarding researchGroupId and reverting the optimistic UI on failures: in the Switch onChange handler capture the previous value (const prev = includeApplicationDataInEmail), check if researchGroupId is defined and if not call showSimpleError/getApiResponseErrorMessage and reset state to prev, then call handleChange; inside handleChange (or the doRequest callback) handle non-ok responses and network errors by calling setIncludeApplicationDataInEmail(prev) to revert the toggle and showSimpleError(getApiResponseErrorMessage(res)) (and add a catch around doRequest to revert on exceptions). Reference symbols: Switch onChange handler, includeApplicationDataInEmail, setIncludeApplicationDataInEmail, handleChange, doRequest, researchGroupId, showSimpleError, getApiResponseErrorMessage.client/src/pages/DataExportPage/DataExportPage.tsx-1-8 (1)
1-8:⚠️ Potential issue | 🟠 MajorUse named export and replace raw HTML div with Mantine component.
Line 194: Change
export default DataExportPagetoexport { DataExportPage }and update imports accordingly.Line 120: Replace the outer
<div>wrapper with a Mantine component like<Stack>or remove it if the inner<Stack>(line 123) suffices.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/pages/DataExportPage/DataExportPage.tsx` around lines 1 - 8, Change the default export to a named export by replacing "export default DataExportPage" with "export { DataExportPage }" and update any imports elsewhere to use the named import; also remove or replace the outer <div> wrapper around the component's JSX (the wrapper around the inner Stack at the top of the DataExportPage render) with a Mantine layout component such as <Stack> (or delete the outer div if the inner <Stack> suffices) so the component uses Mantine primitives consistently and avoids raw HTML containers.client/src/pages/SettingsPage/components/AccountDeletion/AccountDeletion.tsx-80-81 (1)
80-81:⚠️ Potential issue | 🟠 MajorConfirmation bypass when user name is empty.
If
auth.user?.firstNameandauth.user?.lastNameare bothundefined/null,fullNameresolves to""andconfirmNamestarts as"", soconfirmName !== fullNameisfalse— the delete button is enabled without typing anything.🐛 Proposed guard
const fullName = `${auth.user?.firstName ?? ''} ${auth.user?.lastName ?? ''}`.trim() const canDelete = !preview.hasActiveTheses && !preview.isResearchGroupHead + const hasValidName = fullName.length > 0 ... - <Button color='red' disabled={confirmName !== fullName} onClick={onDelete}> + <Button color='red' disabled={!hasValidName || confirmName !== fullName} onClick={onDelete}>Also applies to: 161-161
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/pages/SettingsPage/components/AccountDeletion/AccountDeletion.tsx` around lines 80 - 81, The delete confirmation can be bypassed when fullName is empty; update the logic around fullName, confirmName and canDelete so deletion requires an explicit non-empty match: ensure fullName (computed in AccountDeletion as fullName) is treated as empty when both names missing and include a guard like fullName.length > 0 in the canDelete condition (i.e., only allow deletion if !preview.hasActiveTheses && !preview.isResearchGroupHead && fullName !== '' && confirmName === fullName), or alternatively require a fixed token (e.g., typing "DELETE") when fullName is empty; adjust any UI/initial state for confirmName accordingly so an empty string does not satisfy the confirmation.server/src/main/java/de/tum/cit/aet/thesis/service/UserDeletionService.java-157-170 (1)
157-170:⚠️ Potential issue | 🟠 MajorEntire deferred-deletion batch runs in a single
@Transactional— one failure rolls back all.If
performFullDeletionthrows for one user, the whole batch is rolled back. Consider per-user transaction boundaries (e.g., injectselfviaApplicationContextand call a@Transactional(propagation = REQUIRES_NEW)method, or useTransactionTemplate).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/src/main/java/de/tum/cit/aet/thesis/service/UserDeletionService.java` around lines 157 - 170, processDeferredDeletions currently runs the entire batch in one `@Transactional` so a failure in performFullDeletion for one user rolls back all; change to per-user transaction boundaries by moving the deletion work into a separate method annotated with `@Transactional`(propagation = REQUIRES_NEW) (e.g., create performFullDeletionTransactional or similar) and invoke it via the Spring proxy (inject self via ApplicationContext or a self bean) or use TransactionTemplate to execute each user's deleteAllUserFiles, deleteDataExports and performFullDeletion inside its own new transaction after confirming getRetentionBlockedThesisRoles is empty; ensure processDeferredDeletions loops and calls the proxied/new-transaction method so failures for one user do not rollback others.server/src/main/java/de/tum/cit/aet/thesis/service/DataExportService.java-382-401 (1)
382-401:⚠️ Potential issue | 🟠 MajorInputStream leak in
addUserFile— stream fromresource.getInputStream()is never closed.
FileSystemResource.getInputStream()opens a newFileInputStreameach time.transferTodoes not close the source stream. This leaks file handles, which can exhaust the OS limit during large exports.Proposed fix
private void addUserFile(ZipOutputStream zos, String filename, String entryPrefix) { if (filename == null || filename.isBlank()) { return; } try { FileSystemResource resource = uploadService.load(filename); if (resource.exists()) { String extension = ""; int dotIndex = filename.lastIndexOf('.'); if (dotIndex >= 0) { extension = filename.substring(dotIndex); } zos.putNextEntry(new ZipEntry(entryPrefix + extension)); - resource.getInputStream().transferTo(zos); + try (var is = resource.getInputStream()) { + is.transferTo(zos); + } zos.closeEntry(); } } catch (Exception e) { log.warn("Failed to include file {} in export: {}", filename, e.getMessage()); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/src/main/java/de/tum/cit/aet/thesis/service/DataExportService.java` around lines 382 - 401, The addUserFile method leaks the InputStream from resource.getInputStream() because transferTo doesn't close the source; wrap the InputStream in a try-with-resources (e.g., try (InputStream in = resource.getInputStream()) { ... }) around the transferTo call so the stream is always closed, ensure you still call zos.putNextEntry(entryPrefix + extension) before transfer and zos.closeEntry() after transfer (in a finally or after the try-with-resources) and avoid closing the ZipOutputStream itself; update addUserFile to obtain the InputStream via try-with-resources and perform transferTo(zos) inside that block so file handles are released.server/src/main/java/de/tum/cit/aet/thesis/service/UserDeletionService.java-41-41 (1)
41-41:⚠️ Potential issue | 🟠 Major
DateTimeFormatterwithout explicitLocale— month names will vary by JVM locale.
"MMMM d, yyyy"produces locale-dependent month names (e.g., "Januar" on a German JVM). Since this is for user-facing messages, pin a locale:- private static final DateTimeFormatter DATE_FORMATTER = DateTimeFormatter.ofPattern("MMMM d, yyyy"); + private static final DateTimeFormatter DATE_FORMATTER = DateTimeFormatter.ofPattern("MMMM d, yyyy", Locale.ENGLISH);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/src/main/java/de/tum/cit/aet/thesis/service/UserDeletionService.java` at line 41, The DateTimeFormatter DATE_FORMATTER in UserDeletionService is created without an explicit Locale so month names will vary by JVM locale; update the DATE_FORMATTER initialization to supply an explicit Locale (e.g., Locale.ENGLISH or Locale.US) when calling DateTimeFormatter.ofPattern("MMMM d, yyyy") so the month names are deterministic for user-facing messages.server/src/main/java/de/tum/cit/aet/thesis/service/ResearchGroupService.java-298-298 (1)
298-298:⚠️ Potential issue | 🟠 MajorReference equality (
!=) used to compare JPA entities — use ID-based comparison instead.
oldHead != headcompares object identity. If the same user is loaded from different Hibernate sessions or detached/reattached, two objects representing the same row can be different references. Compare by ID:Proposed fix
- if (oldHead != head) { + if (!oldHead.getId().equals(head.getId())) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/src/main/java/de/tum/cit/aet/thesis/service/ResearchGroupService.java` at line 298, The check in ResearchGroupService that uses reference comparison (oldHead != head) must be replaced with an ID-based equality check to handle detached or reloaded JPA entities: compare the primary key values (e.g., Objects.equals(oldHead.getId(), head.getId())) and handle nulls (oldHead or head may be null) so the logic triggers only when the persisted identity truly changed; update the condition in the method containing oldHead/head to use Objects.equals on IDs (and add an import for java.util.Objects if needed).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (110)
.github/workflows/deploy_docker.yml.gitignoreREADME.mdclient/e2e/account-deletion.spec.tsclient/e2e/auth.setup.tsclient/e2e/data-export.spec.tsclient/e2e/data-retention.spec.tsclient/e2e/research-groups.spec.tsclient/public/generate-runtime-env.jsclient/public/privacy.htmlclient/src/app/Routes.tsxclient/src/app/layout/AuthenticatedArea/AuthenticatedArea.tsxclient/src/components/ApplicationDeleteButton/ApplicationDeleteButton.tsxclient/src/components/UserInformationForm/components/AvatarInput/AvatarInput.tsxclient/src/config/global.tsclient/src/config/types.tsclient/src/pages/AdminPage/AdminPage.tsxclient/src/pages/DataExportPage/DataExportPage.tsxclient/src/pages/InterviewTopicOverviewPage/components/CalendarCarousel.tsxclient/src/pages/PresentationOverviewPage/PresentationOverviewPage.tsxclient/src/pages/PrivacyPage/PrivacyPage.tsxclient/src/pages/ResearchGroupSettingPage/ResearchGroupSettingPage.tsxclient/src/pages/ResearchGroupSettingPage/components/ApplicationEmailContentSettingsCard.tsxclient/src/pages/ResearchGroupSettingPage/components/ScientificWritingGuideSettingsCard.tsxclient/src/pages/ReviewApplicationPage/ReviewApplicationPage.tsxclient/src/pages/ReviewApplicationPage/components/ApplicationReviewBody/ApplicationReviewBody.tsxclient/src/pages/SettingsPage/SettingsPage.tsxclient/src/pages/SettingsPage/components/AccountDeletion/AccountDeletion.tsxclient/src/providers/AuthenticationContext/AuthenticationProvider.tsxclient/src/providers/AuthenticationContext/context.tsclient/src/requests/responses/researchGroupSettings.tsclient/src/utils/user.tsdocker-compose.prod.ymldocker-compose.ymldocs/CONFIGURATION.mddocs/DATA_RETENTION.mddocs/MAILS.mddocs/PRODUCTION.mdexecute-e2e-local.shkeycloak/thesis-management-realm.jsonserver/src/main/java/de/tum/cit/aet/thesis/constants/DataExportState.javaserver/src/main/java/de/tum/cit/aet/thesis/controller/ApplicationController.javaserver/src/main/java/de/tum/cit/aet/thesis/controller/DataExportController.javaserver/src/main/java/de/tum/cit/aet/thesis/controller/DataRetentionController.javaserver/src/main/java/de/tum/cit/aet/thesis/controller/ResearchGroupSettingsController.javaserver/src/main/java/de/tum/cit/aet/thesis/controller/UserDeletionController.javaserver/src/main/java/de/tum/cit/aet/thesis/controller/UserInfoController.javaserver/src/main/java/de/tum/cit/aet/thesis/controller/payload/UpdateResearchGroupSettingsPayload.javaserver/src/main/java/de/tum/cit/aet/thesis/cron/AutomaticRejects.javaserver/src/main/java/de/tum/cit/aet/thesis/cron/ProfilePictureMigration.javaserver/src/main/java/de/tum/cit/aet/thesis/dto/DataExportDto.javaserver/src/main/java/de/tum/cit/aet/thesis/dto/DataRetentionResultDto.javaserver/src/main/java/de/tum/cit/aet/thesis/dto/ResearchGroupSettingsApplicationEmailDTO.javaserver/src/main/java/de/tum/cit/aet/thesis/dto/ResearchGroupSettingsDTO.javaserver/src/main/java/de/tum/cit/aet/thesis/dto/ResearchGroupSettingsWritingGuideDTO.javaserver/src/main/java/de/tum/cit/aet/thesis/dto/UserDeletionPreviewDto.javaserver/src/main/java/de/tum/cit/aet/thesis/dto/UserDeletionResultDto.javaserver/src/main/java/de/tum/cit/aet/thesis/entity/DataExport.javaserver/src/main/java/de/tum/cit/aet/thesis/entity/ResearchGroupSettings.javaserver/src/main/java/de/tum/cit/aet/thesis/entity/ThesisPresentation.javaserver/src/main/java/de/tum/cit/aet/thesis/entity/User.javaserver/src/main/java/de/tum/cit/aet/thesis/exception/CalendarException.javaserver/src/main/java/de/tum/cit/aet/thesis/repository/ApplicationRepository.javaserver/src/main/java/de/tum/cit/aet/thesis/repository/DataExportRepository.javaserver/src/main/java/de/tum/cit/aet/thesis/repository/ResearchGroupRepository.javaserver/src/main/java/de/tum/cit/aet/thesis/repository/ThesisRepository.javaserver/src/main/java/de/tum/cit/aet/thesis/repository/ThesisRoleRepository.javaserver/src/main/java/de/tum/cit/aet/thesis/repository/TopicRoleRepository.javaserver/src/main/java/de/tum/cit/aet/thesis/repository/UserRepository.javaserver/src/main/java/de/tum/cit/aet/thesis/service/AccessManagementService.javaserver/src/main/java/de/tum/cit/aet/thesis/service/ApplicationService.javaserver/src/main/java/de/tum/cit/aet/thesis/service/AuthenticationService.javaserver/src/main/java/de/tum/cit/aet/thesis/service/CalendarService.javaserver/src/main/java/de/tum/cit/aet/thesis/service/DashboardService.javaserver/src/main/java/de/tum/cit/aet/thesis/service/DataExportService.javaserver/src/main/java/de/tum/cit/aet/thesis/service/DataRetentionService.javaserver/src/main/java/de/tum/cit/aet/thesis/service/EmailTemplateService.javaserver/src/main/java/de/tum/cit/aet/thesis/service/GravatarService.javaserver/src/main/java/de/tum/cit/aet/thesis/service/MailingService.javaserver/src/main/java/de/tum/cit/aet/thesis/service/ResearchGroupService.javaserver/src/main/java/de/tum/cit/aet/thesis/service/ThesisCommentService.javaserver/src/main/java/de/tum/cit/aet/thesis/service/ThesisPresentationService.javaserver/src/main/java/de/tum/cit/aet/thesis/service/ThesisService.javaserver/src/main/java/de/tum/cit/aet/thesis/service/TopicService.javaserver/src/main/java/de/tum/cit/aet/thesis/service/UploadService.javaserver/src/main/java/de/tum/cit/aet/thesis/service/UserDeletionService.javaserver/src/main/java/de/tum/cit/aet/thesis/utility/MailConfig.javaserver/src/main/resources/application.ymlserver/src/main/resources/db/changelog/changes/23_seed_dev_test_data.xmlserver/src/main/resources/db/changelog/changes/25_drop_calendar_event_column.sqlserver/src/main/resources/db/changelog/changes/26_add_scientific_writing_guide_link_to_settings.sqlserver/src/main/resources/db/changelog/changes/27_data_retention_preparation.sqlserver/src/main/resources/db/changelog/changes/28_data_export.sqlserver/src/main/resources/db/changelog/changes/29_user_disabled_flag.sqlserver/src/main/resources/db/changelog/changes/30_user_deletion.sqlserver/src/main/resources/db/changelog/changes/31_include_application_data_in_email.sqlserver/src/main/resources/db/changelog/db.changelog-master.xmlserver/src/main/resources/db/changelog/manual/seed_dev_test_data.sqlserver/src/test/java/de/tum/cit/aet/thesis/controller/DashboardControllerTest.javaserver/src/test/java/de/tum/cit/aet/thesis/controller/DataExportControllerTest.javaserver/src/test/java/de/tum/cit/aet/thesis/controller/ResearchGroupSettingsControllerTest.javaserver/src/test/java/de/tum/cit/aet/thesis/mock/BaseIntegrationTest.javaserver/src/test/java/de/tum/cit/aet/thesis/service/CalendarServiceDisabledTest.javaserver/src/test/java/de/tum/cit/aet/thesis/service/CalendarServiceIntegrationTest.javaserver/src/test/java/de/tum/cit/aet/thesis/service/DataRetentionServiceTest.javaserver/src/test/java/de/tum/cit/aet/thesis/service/MailingServiceIntegrationTest.javaserver/src/test/java/de/tum/cit/aet/thesis/service/ThesisServiceTest.javaserver/src/test/java/de/tum/cit/aet/thesis/service/UserDeletionServiceTest.javaserver/src/test/resources/application.ymlserver/src/test/resources/testcontainers.properties
💤 Files with no reviewable changes (10)
- docker-compose.prod.yml
- docker-compose.yml
- client/src/config/types.ts
- server/src/main/java/de/tum/cit/aet/thesis/utility/MailConfig.java
- server/src/test/java/de/tum/cit/aet/thesis/service/CalendarServiceDisabledTest.java
- server/src/main/java/de/tum/cit/aet/thesis/exception/CalendarException.java
- server/src/main/java/de/tum/cit/aet/thesis/entity/ThesisPresentation.java
- client/src/config/global.ts
- client/public/generate-runtime-env.js
- server/src/test/java/de/tum/cit/aet/thesis/service/CalendarServiceIntegrationTest.java
server/src/main/java/de/tum/cit/aet/thesis/repository/ResearchGroupRepository.java
Show resolved
Hide resolved
server/src/main/java/de/tum/cit/aet/thesis/service/UserDeletionService.java
Show resolved
Hide resolved
…onal - P0: Use thesis completion date (FINISHED/DROPPED_OUT state change) instead of createdAt for retention expiry calculation, with createdAt as fallback - P1: Add path traversal protection to UploadService.deleteFile (check for .. and validate resolved path stays within root directory) - P1: Add path validation to data export file deletion (verify path is within the configured export directory before deleting) - P1: Remove @transactional from UserDeletionService — file deletions now happen after all DB operations succeed (worst case: orphaned files, not lost data). All delete operations use JPQL via @Modifying @transactional on repository methods to avoid Hibernate lazy initialization issues - Add JPQL delete methods to repositories: NotificationSettingRepository, UserGroupRepository, UserRepository, ThesisRoleRepository, TopicRoleRepository, ApplicationReviewerRepository Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
client/src/components/UserMultiSelect/UserMultiSelect.tsx (2)
42-79:⚠️ Potential issue | 🟡 MinorAvoid stale
selectedvalues in the async response.The effect captures
selectedat request time; if selection changes while the request is in flight, the response will use the outdated closure value and filter out newly selected items. Use a ref to always read the current selection.✅ Keep selection current with useRef
-import { useEffect, useState } from 'react' +import { useEffect, useRef, useState } from 'react' ... const selected: string[] = inputProps.value ?? [] + const selectedRef = useRef<string[]>(selected) + useEffect(() => { + selectedRef.current = selected + }, [selected]) ... - ...prevState.filter((item) => selected.includes(item.value)), + ...prevState.filter((item) => selectedRef.current.includes(item.value)),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/components/UserMultiSelect/UserMultiSelect.tsx` around lines 42 - 79, The effect's async response closes over the stale selected variable causing newly selected items to be dropped; create a ref (e.g., selectedRef = useRef(selected)) and update it whenever selected changes (another small useEffect or inside the selection setter) then in the doRequest success handler use selectedRef.current instead of selected when filtering prevState in the setData call (the arrayUnique/filter logic inside the useEffect/doRequest response). Ensure the ref is kept in sync so the response always uses the latest selection.
1-12:⚠️ Potential issue | 🟠 MajorFix stale closure on
selectedin useEffect dependency array.The
selectedvariable is used in the filter on line 62 but missing from the dependency array on line 79. This causes stale closure—whengroupsor search value changes, the effect re-runs butselectedreferences an outdated value, incorrectly filtering previously selected items. Addselectedto the dependency array.Also address implicit
anytypes in callback parameters (lines 59, 62, 98, 115) per coding guidelines. Consider typingprevStateand destructured params explicitly.ESLint could not run due to missing dependencies in the environment; ensure these checks pass locally before merging.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/components/UserMultiSelect/UserMultiSelect.tsx` around lines 1 - 12, The effect using useEffect that filters groups references the selected variable but doesn't include selected in its dependency array—add selected to the dependency array of that useEffect so the filter uses the latest selected value. Also fix implicit any types in all inline callbacks noted (the filter/map callback where selected is used, setGroups(prevState => ...), and any renderItem/item label callbacks) by explicitly typing parameters (e.g., prevState: ILightUser[], item: ILightUser or the appropriate shape) to satisfy linting; update signatures for functions referenced like setGroups, the filter using selected, and renderItem to use explicit types. Ensure ESLint/type checks pass locally after these changes.
🧹 Nitpick comments (3)
client/playwright.config.ts (1)
8-8: Simplify the redundant ternary — both branches are identical.
process.env.CI ? 2 : 2always evaluates to2. The conditional adds noise without any behavioral difference.♻️ Proposed simplification
- workers: process.env.CI ? 2 : 2, + workers: 2,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/playwright.config.ts` at line 8, The workers setting uses a redundant ternary expression (process.env.CI ? 2 : 2) which always evaluates to 2; simplify the Playwright config by replacing that expression with a plain numeric assignment (workers: 2) and remove the unnecessary conditional to reduce noise while keeping behavior identical, locating the change at the workers property in client/playwright.config.ts.client/e2e/application-review-workflow.spec.ts (1)
11-14: Silent pass on load failure in a serial suite could mask cascading issues.In a serial suite, if the first test silently returns because
navigateToDetailfails, the second test likely also skips silently. Both tests "pass" but nothing was actually verified. Consider logging a warning or usingtest.skip()instead ofreturnso the test report surfaces that these were not actually exercised.Suggested approach
const loaded = await navigateToDetail(page, `/applications/${APPLICATION_REJECT_ID}`, heading) - if (!loaded) return // Application not accessible (may have been modified by a parallel test) + if (!loaded) { + test.skip(true, 'Application detail page did not load — skipping') + return + }Also applies to: 51-54
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/e2e/application-review-workflow.spec.ts` around lines 11 - 14, The test silently returning when navigateToDetail(...) fails hides skipped work; replace the early "if (!loaded) return" with a visible skip so the test runner reports it—call test.skip(true, `navigateToDetail failed for ${APPLICATION_REJECT_ID}`) (or test.skip(true, 'navigateToDetail failed') if string interpolation is inconvenient) when loaded is false, then return; update the same pattern around the other occurrence (the block using navigateToDetail, heading, and APPLICATION_REJECT_ID at lines ~51-54) so failures are surfaced instead of silently passing.client/src/components/UserMultiSelect/UserMultiSelect.tsx (1)
4-5: Use type-only imports for DTO types.These imports are only used in type positions; switch to
import typeto match TS best practices and repo rules.♻️ Suggested change
-import { PaginationResponse } from '../../requests/responses/pagination' -import { ILightUser } from '../../requests/responses/user' +import type { PaginationResponse } from '../../requests/responses/pagination' +import type { ILightUser } from '../../requests/responses/user'As per coding guidelines, Use 'import type' for type-only imports.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/components/UserMultiSelect/UserMultiSelect.tsx` around lines 4 - 5, The imports PaginationResponse and ILightUser in UserMultiSelect.tsx are used only as types; change their declarations to use TypeScript's type-only import form (e.g., "import type { PaginationResponse } ..." and "import type { ILightUser } ...") so the compiler and bundler know they're erased at runtime; update the import line importing PaginationResponse and ILightUser accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/e2e/data-retention.spec.ts`:
- Around line 18-24: The current check uses heading.isVisible({ timeout: 30_000
}) which may resolve instantly and cause a false negative; replace this with an
explicit wait that actually polls until the heading is present and visible
(e.g., use Playwright's waitForSelector/waitForFunction or
expect(...).toBeVisible) instead of isVisible. Update the code that references
heading and isVisible in data-retention.spec.ts to call a waiting API (e.g.,
page.waitForSelector with the same role/name or expect(heading).toBeVisible with
a timeout) and only set hasApplication = false if the wait throws or times out,
preserving the graceful skip behavior when the application is truly missing.
---
Outside diff comments:
In `@client/src/components/UserMultiSelect/UserMultiSelect.tsx`:
- Around line 42-79: The effect's async response closes over the stale selected
variable causing newly selected items to be dropped; create a ref (e.g.,
selectedRef = useRef(selected)) and update it whenever selected changes (another
small useEffect or inside the selection setter) then in the doRequest success
handler use selectedRef.current instead of selected when filtering prevState in
the setData call (the arrayUnique/filter logic inside the useEffect/doRequest
response). Ensure the ref is kept in sync so the response always uses the latest
selection.
- Around line 1-12: The effect using useEffect that filters groups references
the selected variable but doesn't include selected in its dependency array—add
selected to the dependency array of that useEffect so the filter uses the latest
selected value. Also fix implicit any types in all inline callbacks noted (the
filter/map callback where selected is used, setGroups(prevState => ...), and any
renderItem/item label callbacks) by explicitly typing parameters (e.g.,
prevState: ILightUser[], item: ILightUser or the appropriate shape) to satisfy
linting; update signatures for functions referenced like setGroups, the filter
using selected, and renderItem to use explicit types. Ensure ESLint/type checks
pass locally after these changes.
---
Nitpick comments:
In `@client/e2e/application-review-workflow.spec.ts`:
- Around line 11-14: The test silently returning when navigateToDetail(...)
fails hides skipped work; replace the early "if (!loaded) return" with a visible
skip so the test runner reports it—call test.skip(true, `navigateToDetail failed
for ${APPLICATION_REJECT_ID}`) (or test.skip(true, 'navigateToDetail failed') if
string interpolation is inconvenient) when loaded is false, then return; update
the same pattern around the other occurrence (the block using navigateToDetail,
heading, and APPLICATION_REJECT_ID at lines ~51-54) so failures are surfaced
instead of silently passing.
In `@client/playwright.config.ts`:
- Line 8: The workers setting uses a redundant ternary expression
(process.env.CI ? 2 : 2) which always evaluates to 2; simplify the Playwright
config by replacing that expression with a plain numeric assignment (workers: 2)
and remove the unnecessary conditional to reduce noise while keeping behavior
identical, locating the change at the workers property in
client/playwright.config.ts.
In `@client/src/components/UserMultiSelect/UserMultiSelect.tsx`:
- Around line 4-5: The imports PaginationResponse and ILightUser in
UserMultiSelect.tsx are used only as types; change their declarations to use
TypeScript's type-only import form (e.g., "import type { PaginationResponse }
..." and "import type { ILightUser } ...") so the compiler and bundler know
they're erased at runtime; update the import line importing PaginationResponse
and ILightUser accordingly.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
client/e2e/application-review-workflow.spec.tsclient/e2e/data-retention.spec.tsclient/e2e/helpers.tsclient/e2e/proposal-feedback-workflow.spec.tsclient/e2e/thesis-grading-workflow.spec.tsclient/e2e/thesis-workflow.spec.tsclient/e2e/topic-workflow.spec.tsclient/playwright.config.tsclient/src/components/UserMultiSelect/UserMultiSelect.tsx
- Add error isolation in nightly cleanup (each step caught independently) - Add per-user error handling in deferred deletion batch loop - Add path traversal protection in deleteExpiredExports - Fix LazyInitializationException: make createDataExport @transactional - Fix getBytes() to use StandardCharsets.UTF_8 - Use saveAll() instead of N individual save() calls - Remove unused deleteUserById repository method - Remove TODO comments from production code - Fix E2E isVisible to use waitFor for proper timeout behavior Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
client/e2e/navigation.spec.ts (1)
72-72: Timeout inconsistency: 30 s vs 15 s used everywhere else in this file.Every other page-level assertion here uses
{ timeout: 15_000 }. Bumping only this one to30_000silently accepts that the back-navigation to/dashboardis twice as slow as any other transition, which could mask a real performance regression introduced by this PR's new features (avatar migration, retention settings, etc.). If the higher value is genuinely needed, a brief inline comment explaining why would prevent it from being cargo-culted further.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/e2e/navigation.spec.ts` at line 72, The test uses an inconsistent timeout for the page URL assertion (await expect(page).toHaveURL(/\/dashboard/, { timeout: 30_000 })); change it to match the rest of the file by using { timeout: 15_000 } or, if 30_000 is actually required, add a short inline comment explaining why this navigation needs the longer timeout (mentioning page and toHaveURL to locate the assertion).server/src/main/java/de/tum/cit/aet/thesis/repository/UserGroupRepository.java (1)
15-18: Consider adding@Paramfor consistency with the rest of the codebase.The
-parameterscompiler flag is enabled in the build configuration, so Spring Data JPA can bind:userIdwithout explicit@Param. However, most other@Querymethods in the repository layer use@Paramannotations. Adding it here improves consistency and clarity.Optional alignment
import org.springframework.data.jpa.repository.JpaRepository; import org.springframework.data.jpa.repository.Modifying; import org.springframework.data.jpa.repository.Query; +import org.springframework.data.repository.query.Param; import org.springframework.stereotype.Repository; import org.springframework.transaction.annotation.Transactional; @@ - void deleteByUserId(UUID userId); + void deleteByUserId(`@Param`("userId") UUID userId);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/src/main/java/de/tum/cit/aet/thesis/repository/UserGroupRepository.java` around lines 15 - 18, The deleteByUserId method declaration lacks an explicit `@Param` on its UUID parameter, which is inconsistent with other repository queries; update the method signature for deleteByUserId(UUID userId) to annotate the parameter with `@Param`("userId") (and add the import if missing) so the `@Query`(":userId") binding is explicit and consistent with the rest of the repository.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@client/e2e/navigation.spec.ts`:
- Line 72: The test uses an inconsistent timeout for the page URL assertion
(await expect(page).toHaveURL(/\/dashboard/, { timeout: 30_000 })); change it to
match the rest of the file by using { timeout: 15_000 } or, if 30_000 is
actually required, add a short inline comment explaining why this navigation
needs the longer timeout (mentioning page and toHaveURL to locate the
assertion).
In
`@server/src/main/java/de/tum/cit/aet/thesis/repository/UserGroupRepository.java`:
- Around line 15-18: The deleteByUserId method declaration lacks an explicit
`@Param` on its UUID parameter, which is inconsistent with other repository
queries; update the method signature for deleteByUserId(UUID userId) to annotate
the parameter with `@Param`("userId") (and add the import if missing) so the
`@Query`(":userId") binding is explicit and consistent with the rest of the
repository.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
client/e2e/application-workflow.spec.tsclient/e2e/applications.spec.tsclient/e2e/helpers.tsclient/e2e/navigation.spec.tsclient/e2e/theses.spec.tsclient/e2e/thesis-workflow.spec.tsclient/e2e/topic-workflow.spec.tsclient/src/components/UserMultiSelect/UserMultiSelect.tsxserver/src/main/java/de/tum/cit/aet/thesis/repository/UserGroupRepository.java
🚧 Files skipped from review as they are similar to previous changes (2)
- client/e2e/thesis-workflow.spec.ts
- client/src/components/UserMultiSelect/UserMultiSelect.tsx
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
client/e2e/data-retention.spec.ts (1)
18-24: Prefer explicit test skip over silent return.A bare
returnmakes the test pass, which can mask missing fixtures. Usetest.skipso the report shows a skip with reason.🔧 Suggested change
- if (!hasApplication) { - // Application was already deleted in a prior run — skip gracefully - return - } + if (!hasApplication) { + // Application was already deleted in a prior run — skip gracefully + test.skip(true, 'Application already deleted in a prior run') + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/e2e/data-retention.spec.ts` around lines 18 - 24, The code silently returns when the application heading isn't found (variables: heading, hasApplication, page.getByRole), which masks missing fixtures; change the behavior to explicitly skip the test by calling test.skip when hasApplication is false (e.g., await the waitFor as now to set hasApplication, then replace the bare return with test.skip(!hasApplication, 'Application was already deleted in a prior run') so the test runner reports a skipped test rather than a false pass).server/src/main/java/de/tum/cit/aet/thesis/repository/UserRepository.java (1)
81-103: Consider indexing for nightly cleanup performance.
This query is likely run in batch cleanup; ensure indexes exist on UserGroup.userId, ThesisRole.user.id, and Application.user.id to keep it fast at scale.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/src/main/java/de/tum/cit/aet/thesis/repository/UserRepository.java` around lines 81 - 103, The query in findInactiveStudentCandidates may be slow during nightly cleanup because it scans joins and NOT EXISTS on related tables; add database indexes on the referenced foreign/key columns to speed those checks — specifically create/index UserGroup.id.userId (or UserGroup.user_id), ThesisRole.user.id (or thesis_role.user_id), and Application.user.id (or application.user_id); also ensure indexes on User.lastLoginAt, User.updatedAt (or their persisted column names) if filtering by cutoff is frequent; update the migration or schema changes accordingly so the DB has these indexes before running the batch job.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/src/main/java/de/tum/cit/aet/thesis/service/UserDeletionService.java`:
- Around line 123-164: The avatar file is cleared by performSoftDeletion(), so
calling uploadService.deleteFile(user.getAvatar()) afterward becomes a no-op and
leaks the file; fix by capturing the avatar path (e.g., String avatarPath =
user.getAvatar()) before calling performSoftDeletion() or by invoking
uploadService.deleteFile(...) before performSoftDeletion(), then perform the
soft delete, and ensure you null-check the captured avatarPath before calling
uploadService.deleteFile to avoid NPEs; update the deleteOrAnonymizeUser flow
accordingly (references: deleteOrAnonymizeUser, performSoftDeletion,
uploadService.deleteFile).
---
Nitpick comments:
In `@client/e2e/data-retention.spec.ts`:
- Around line 18-24: The code silently returns when the application heading
isn't found (variables: heading, hasApplication, page.getByRole), which masks
missing fixtures; change the behavior to explicitly skip the test by calling
test.skip when hasApplication is false (e.g., await the waitFor as now to set
hasApplication, then replace the bare return with test.skip(!hasApplication,
'Application was already deleted in a prior run') so the test runner reports a
skipped test rather than a false pass).
In `@server/src/main/java/de/tum/cit/aet/thesis/repository/UserRepository.java`:
- Around line 81-103: The query in findInactiveStudentCandidates may be slow
during nightly cleanup because it scans joins and NOT EXISTS on related tables;
add database indexes on the referenced foreign/key columns to speed those checks
— specifically create/index UserGroup.id.userId (or UserGroup.user_id),
ThesisRole.user.id (or thesis_role.user_id), and Application.user.id (or
application.user_id); also ensure indexes on User.lastLoginAt, User.updatedAt
(or their persisted column names) if filtering by cutoff is frequent; update the
migration or schema changes accordingly so the DB has these indexes before
running the batch job.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
client/e2e/data-retention.spec.tsserver/src/main/java/de/tum/cit/aet/thesis/repository/UserRepository.javaserver/src/main/java/de/tum/cit/aet/thesis/service/DataExportService.javaserver/src/main/java/de/tum/cit/aet/thesis/service/DataRetentionService.javaserver/src/main/java/de/tum/cit/aet/thesis/service/UserDeletionService.java
🚧 Files skipped from review as they are similar to previous changes (1)
- server/src/main/java/de/tum/cit/aet/thesis/service/DataRetentionService.java
server/src/main/java/de/tum/cit/aet/thesis/service/UserDeletionService.java
Show resolved
Hide resolved
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
General: url input validation incorrect. Accepts e.g. 'https://someurlcom' which should be Unable to verify cron job related tasks (see second case) |
performSoftDeletion() nulls the avatar field, so the subsequent deleteFile(user.getAvatar()) was a no-op leaving orphaned files on disk. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Member
Author
|
Fixed the avatar file deletion bug flagged by CodeRabbit: captured the avatar path before |
1. Add @transactional to deleteOrAnonymizeUser to ensure atomicity across multiple DB operations (prevents partial deletion on failure) 2. Fix stale entity in DataExportService error handler — re-fetch after JPQL claimForProcessing bypasses the persistence context 3. Move @transactional from self-invoked createDataExport (where Spring AOP cannot intercept it) to processAllPendingExports, and make createDataExport private 4. Delete old avatar file in importProfilePicture before storing new one to prevent orphaned files on disk 5. Reuse HttpClient in GravatarService instead of creating a new instance per request (avoids resource waste during migration) 6. Add logging to UploadService.deleteFile so failed file deletions are not silently swallowed Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…dration error 1. Revert @transactional on deleteOrAnonymizeUser and processAllPendingExports — per project convention, services should not use @transactional due to performance/concurrency concerns 2. Document the @transactional avoidance policy in CLAUDE.md and docs/DEVELOPMENT.md 3. Move data export UI into the Account tab in Settings, replacing the standalone /data-export route with a redirect 4. Fix React hydration error in DocumentEditor: InputError renders a <p> element, and the error content was a <Group> (<div>), causing invalid <div>-inside-<p> nesting. Replaced with inline <span> elements and only render when there is content to show. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Member
Author
The admin UI does not support to configure the retention period
I am not sure where you experienced this, but this seems to be unrelated to this particular PR |
krusche
added a commit
that referenced
this pull request
Feb 24, 2026
These methods belong to PR #864 and were accidentally included here. Removing them to keep this PR focused and avoid merge conflicts. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Move Flex outside of Text to avoid nesting a div inside a p element, which caused a React hydration error in the browser console. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
As the system is now used by multiple research groups (beyond just a single group), this PR improves data privacy practices and adds per-group configurability to better serve diverse institutional needs.
Test plan
/privacyserver/src/main/resources/application.yml-->thesis-management.data-retention.cronto e.g. every minute${DATA_RETENTION_CRON:0 * * * * *}MailLoggerwhich is only active in dev environments)./execute-e2e-local.shcd server && ./gradlew testSummary by CodeRabbit
New Features
Documentation