eeg_portal/docs/compose/plans/code-review-critical-fixes.md
Bernhard Müller cfc32b3a7d chore: cleanup test data and add missing test files
- Add TariffInviteServiceTest, GlobalExceptionHandlerTest, setup-test.ts
- Add compose plans/specs docs
- Extend .gitignore with *.xlsx, *.ps1, *.http, *.py, *.docx
- Remove stale test data files and scripts from working tree
2026-07-24 11:48:56 +02:00

469 lines
18 KiB
Markdown
Raw Permalink Blame History

This file contains ambiguous Unicode characters

This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.

# Plan: Code-Review-Fixes — Kritische und hohe Befunde
## Task-Übersicht
| ID | Beschreibung | Schwere | Abhängigkeit |
|----|-------------|---------|--------------|
| T1 | GlobalExceptionHandler: MethodArgumentNotValidException-Handler | KRITISCH | — |
| T2 | GlobalExceptionHandler: AtNumberAlreadyExistsException-Handler fix | HOCH | — |
| T3 | EnergyCommunityAdminController: @PreAuthorize hinzufügen | KRITISCH | — |
| T4 | NotificationController/Service: IDOR-Fix (markAsRead) | KRITISCH | — |
| T5 | AdminIamService.rejectUser: ResponseStatusException → IAE/ISE | HOCH | — |
| T6 | EnergyCommunityService: ResponseStatusException → IAE/ISE | HOCH | — |
| T7 | TariffService: validateUserTariffRequest() extrahieren | HOCH | — |
| T8 | MeteringPoint.memberships: orphanRemoval=true | HOCH | — |
| T9 | application-prod.yml: ddl-auto Default auf validate | HOCH | — |
| T10 | AdminIamServiceTest: rejectUser-Tests aktualisieren | HOCH | T5 |
| T11 | GlobalExceptionHandlerTest: neue Tests schreiben | HOCH | T1, T2 |
| T12 | NotificationServiceTest: Ownership-Check testen | HOCH | T4 |
| T13 | TariffServiceTest: Tests nach Refactoring prüfen | HOCH | T7 |
| T14 | Alle Backend-Tests ausführen | HOCH | T1T13 |
---
## T1: GlobalExceptionHandler — MethodArgumentNotValidException-Handler
**Beschreibung:** Füge einen `@ExceptionHandler(MethodArgumentNotValidException.class)` hinzu, der Spring's `@Valid`-Fehler im konsistenten `ErrorResponse`-Format zurückgibt.
**Dateien:**
- `eeg_backend/src/main/java/at/mueller/eeg/backend/common/exception/GlobalExceptionHandler.java`
**Akzeptanz:**
- [ ] Handler vorhanden mit `@ExceptionHandler(MethodArgumentNotValidException.class)`
- [ ] Extrahiert `BindingResult`-Fehler als komma-separierte Nachricht
- [ ] Gibt 400 BAD_REQUEST mit `ErrorResponse` zurück
- [ ] Logging auf WARN-Level
**Details:**
```java
@ExceptionHandler(MethodArgumentNotValidException.class)
public ResponseEntity<ErrorResponse> handleValidation(MethodArgumentNotValidException ex) {
String message = ex.getBindingResult().getFieldErrors().stream()
.map(fe -> fe.getField() + ": " + fe.getDefaultMessage())
.collect(Collectors.joining(", "));
log.warn("Validation error: {}", message);
return ResponseEntity
.status(HttpStatus.BAD_REQUEST)
.body(new ErrorResponse(message, HttpStatus.BAD_REQUEST.value()));
}
```
Import: `org.springframework.validation.BindException` oder `MethodArgumentNotValidException` + `org.springframework.web.bind.MethodArgumentNotValidException` + `java.util.stream.Collectors`.
---
## T2: GlobalExceptionHandler — AtNumberAlreadyExistsException-Handler fix
**Beschreibung:** Ersetze den hardcoded String durch `ex.getMessage()`.
**Dateien:**
- `eeg_backend/src/main/java/at/mueller/eeg/backend/common/exception/GlobalExceptionHandler.java`
**Akzeptanz:**
- [ ] Zeile 59: `"Zählerpunkt existiert bereits"``ex.getMessage()`
- [ ] Kompiliert ohne Fehler
**Details:**
Vorher:
```java
.body(new ErrorResponse("Zählerpunkt existiert bereits", HttpStatus.CONFLICT.value()));
```
Nachher:
```java
.body(new ErrorResponse(ex.getMessage(), HttpStatus.CONFLICT.value()));
```
---
## T3: EnergyCommunityAdminController — @PreAuthorize hinzufügen
**Beschreibung:** Füge `@PreAuthorize("hasRole('ADMIN')")` auf Klassenebene hinzu, konsistent mit `AdminIamController`.
**Dateien:**
- `eeg_backend/src/main/java/at/mueller/eeg/backend/community/api/EnergyCommunityAdminController.java`
**Akzeptanz:**
- [ ] `@PreAuthorize("hasRole('ADMIN')")` auf Klassenebene (nach `@RequestMapping`)
- [ ] Import für `org.springframework.security.access.prepost.PreAuthorize` vorhanden
- [ ] Kompiliert ohne Fehler
**Details:**
```java
@RestController
@RequestMapping("/api/admin/energy-communities")
@PreAuthorize("hasRole('ADMIN')")
@RequiredArgsConstructor
public class EnergyCommunityAdminController {
```
---
## T4: NotificationController/Service — IDOR-Fix (markAsRead)
**Beschreibung:** Füge `@CurrentUserId` zum Controller hinzu und einen Ownership-Check im Service.
**Dateien:**
- `eeg_backend/src/main/java/at/mueller/eeg/backend/common/api/NotificationController.java`
- `eeg_backend/src/main/java/at/mueller/eeg/backend/common/service/NotificationService.java`
**Akzeptanz:**
- [ ] Controller: `markAsRead(@PathVariable UUID id, @CurrentUserId String userId)`
- [ ] Controller ruft `notificationService.markAsRead(id, UUID.fromString(userId))`
- [ ] Service: `markAsRead(UUID id, UUID userId)` — Ownership-Check
- [ ] Wenn `notification.getUserId().equals(userId)` nicht zutrifft: `AccessDeniedException`
- [ ] Import für `org.springframework.security.access.AccessDeniedException` im Service
- [ ] Kompiliert ohne Fehler
**Details:**
Controller:
```java
@PutMapping("/{id}/read")
@PreAuthorize("hasRole('MEMBER') or hasRole('ADMIN')")
public ResponseEntity<Void> markAsRead(@PathVariable UUID id, @CurrentUserId String userId) {
notificationService.markAsRead(id, UUID.fromString(userId));
return ResponseEntity.ok().build();
}
```
Service:
```java
@Transactional
public void markAsRead(UUID id, UUID userId) {
Notification notification = notificationRepository.findById(id)
.orElseThrow(() -> new IllegalArgumentException("Notification nicht gefunden: " + id));
if (!notification.getUserId().equals(userId)) {
throw new AccessDeniedException("Keine Berechtigung, diese Notification als gelesen zu markieren.");
}
notification.setRead(true);
notificationRepository.save(notification);
}
```
---
## T5: AdminIamService.rejectUser — ResponseStatusException → IAE/ISE
**Beschreibung:** Ersetze `ResponseStatusException` durch `IllegalArgumentException`/`IllegalStateException`, konsistent mit `approveUser`.
**Dateien:**
- `eeg_backend/src/main/java/at/mueller/eeg/backend/iam/service/AdminIamService.java`
**Akzeptanz:**
- [ ] Zeile 56: `ResponseStatusException(NOT_FOUND)``IllegalArgumentException("User nicht gefunden")`
- [ ] Zeile 59: `ResponseStatusException(BAD_REQUEST)``IllegalStateException("Nur User im Status PENDING können abgelehnt werden.")`
- [ ] Importe für `HttpStatus` und `ResponseStatusException` können entfernt werden (prüfen ob noch used)
- [ ] Kompiliert ohne Fehler
**Details:**
```java
@Transactional
public void rejectUser(UUID userId) {
User user = userRepository.findById(userId)
.orElseThrow(() -> new IllegalArgumentException("User nicht gefunden"));
if (user.getStatus() != RegistrationStatus.PENDING) {
throw new IllegalStateException("Nur User im Status PENDING können abgelehnt werden.");
}
user.setStatus(RegistrationStatus.REJECTED);
userRepository.save(user);
eventPublisher.publishEvent(new UserRejectedEvent(user.getId()));
}
```
---
## T6: EnergyCommunityService — ResponseStatusException → IAE/ISE
**Beschreibung:** Ersetze alle `ResponseStatusException` durch `IllegalArgumentException`/`IllegalStateException`.
**Dateien:**
- `eeg_backend/src/main/java/at/mueller/eeg/backend/community/service/EnergyCommunityService.java`
**Akzeptanz:**
- [ ] `update()`: `ResponseStatusException(NOT_FOUND)``IllegalArgumentException("Energiegemeinschaft nicht gefunden: " + id)`
- [ ] `delete()`: `ResponseStatusException(CONFLICT)``IllegalStateException("Die Energiegemeinschaft kann nicht gelöscht werden, da noch Zählpunkte zugeordnet sind.")`
- [ ] Importe für `HttpStatus` und `ResponseStatusException` können entfernt werden
- [ ] Kompiliert ohne Fehler
**Details:**
```java
@Transactional
public EnergyCommunityDto update(UUID id, EnergyCommunityDto dto) {
EnergyCommunity existingEntity = energyCommunityRepository.findById(id)
.orElseThrow(() -> new IllegalArgumentException("Energiegemeinschaft nicht gefunden: " + id));
// ... rest unchanged
}
@Transactional
public void delete(UUID id) {
if (!energyCommunityRepository.existsById(id)) {
throw new IllegalArgumentException("Energy Community mit ID " + id + " existiert nicht.");
}
if (membershipRepository.existsByEnergyCommunityId(id)) {
throw new IllegalStateException("Die Energiegemeinschaft kann nicht gelöscht werden, da noch Zählpunkte zugeordnet sind.");
}
energyCommunityRepository.deleteById(id);
}
```
---
## T7: TariffService — validateUserTariffRequest() extrahieren
**Beschreibung:** Extrahiere die gemeinsame Validierungslogik in eine private Methode.
**Dateien:**
- `eeg_backend/src/main/java/at/mueller/eeg/backend/tariff/service/TariffService.java`
**Akzeptanz:**
- [ ] Private Methode `validateUserTariffRequest(UUID communityId, UserTariffRequest request)` vorhanden
- [ ] Enthält: source != target, Points vorhanden, Points ACTIVE, Community-Tarif vorhanden, Preis <= Max, beide User aktive Members
- [ ] `createUserTariff` ruft `validateUserTariffRequest` auf + Duplicate-Tarif-Check + Invite-Check
- [ ] `updateUserTariff` ruft `validateUserTariffRequest` auf + Ownership-Check (Ownership-Check bleibt vor der Validierung)
- [ ] Alle 25 bestehenden TariffServiceTest weiterhin grün
- [ ] Kompiliert ohne Fehler
**Details:**
Die Methode nimmt `communityId` und `request`, lädt die Points, und wirft Exceptions. Rückgabe: Pair von `(sourcePoint, targetPoint)` odervoid (die Points werden für die Ownership-Check-Logik in updateUserTariff nicht gebraucht, da der Ownership-Check vorher erfolgt).
```java
private void validateUserTariffRequest(UUID communityId, UserTariffRequest request) {
if (request.sourceMeteringPointId().equals(request.targetMeteringPointId())) {
throw new IllegalStateException("Quell- und Ziel-Zählpunkt müssen unterschiedlich sein.");
}
MeteringPoint sourcePoint = meteringPointRepository.findById(request.sourceMeteringPointId())
.orElseThrow(() -> new IllegalArgumentException(
"Quell-Zählpunkt nicht gefunden: " + request.sourceMeteringPointId()));
MeteringPoint targetPoint = meteringPointRepository.findById(request.targetMeteringPointId())
.orElseThrow(() -> new IllegalArgumentException(
"Ziel-Zählpunkt nicht gefunden: " + request.targetMeteringPointId()));
if (sourcePoint.getMakoState() != MakoState.ACTIVE) {
throw new IllegalStateException("Quell-Zählpunkt ist nicht aktiv.");
}
if (targetPoint.getMakoState() != MakoState.ACTIVE) {
throw new IllegalStateException("Ziel-Zählpunkt ist nicht aktiv.");
}
CommunityTariff communityTariff = communityTariffRepository.findByEnergyCommunityId(communityId)
.orElseThrow(() -> new IllegalArgumentException(
"Kein Community-Tarif für Energiegemeinschaft vorhanden: " + communityId));
if (request.pricePerKwhCents().compareTo(communityTariff.getMaxPricePerKwhCents()) > 0) {
throw new IllegalStateException(
"Preis darf den Maximalpreis des Community-Tarifs nicht überschreiten. " +
"Maximalpreis: " + communityTariff.getMaxPricePerKwhCents() + " Cent/kWh");
}
UUID sourceUserId = sourcePoint.getUserId();
UUID targetUserId = targetPoint.getUserId();
if (!membershipRepository.isActiveMemberOfCommunity(sourceUserId, communityId)) {
throw new IllegalStateException("Quell-Benutzer ist kein Mitglied der Energiegemeinschaft.");
}
if (!membershipRepository.isActiveMemberOfCommunity(targetUserId, communityId)) {
throw new IllegalStateException("Ziel-Benutzer ist kein Mitglied der Energiegemeinschaft.");
}
}
```
**Achtung:** `updateUserTariff` muss den Ownership-Check BEIDER Validierung durchführen, da der eigene SourcePoint für den Ownership-Check gebraucht wird. Die Validierungsmethode lädt die Points ohnehin, aber der Ownership-Check muss vorher erfolgen (auf dem ursprünglichen tariff.getSourceMeteringPointId(), nicht auf request.sourceMeteringPointId()).
---
## T8: MeteringPoint.memberships — orphanRemoval=true
**Beschreibung:** Füge `orphanRemoval = true` zum `@OneToMany`-Mapping hinzu.
**Dateien:**
- `eeg_backend/src/main/java/at/mueller/eeg/backend/community/domain/MeteringPoint.java`
**Akzeptanz:**
- [ ] Zeile 51: `cascade = CascadeType.ALL``cascade = CascadeType.ALL, orphanRemoval = true`
- [ ] Kompiliert ohne Fehler
**Details:**
```java
@OneToMany(mappedBy = "meteringPoint", cascade = CascadeType.ALL, orphanRemoval = true)
private List<Membership> memberships;
```
---
## T9: application-prod.yml — ddl-auto Default auf validate
**Beschreibung:** Ändere den Default-Wert für `ddl-auto` von `update` auf `validate`.
**Dateien:**
- `eeg_backend/src/main/resources/application-prod.yml`
**Akzeptanz:**
- [ ] Zeile 9: `ddl-auto: ${JPA_DDL_AUTO:update}``ddl-auto: ${JPA_DDL_AUTO:validate}`
- [ ] Environment-Variable `JPA_DDL_AUTO` kann weiterhin überschreiben
**Details:**
```yaml
ddl-auto: ${JPA_DDL_AUTO:validate}
```
---
## T10: AdminIamServiceTest — rejectUser-Tests aktualisieren
**Beschreibung:** Aktualisiere die Tests für `rejectUser`, da die Exception-Typen sich ändern (ResponseStatusException → IllegalArgumentException/IllegalStateException).
**Dateien:**
- `eeg_backend/src/test/java/at/mueller/eeg/backend/iam/service/AdminIamServiceTest.java`
**Akzeptanz:**
- [ ] `rejectUser_notPending_throws()`: `assertThrows(ResponseStatusException.class, ...)``assertThrows(IllegalStateException.class, ...)`
- [ ] `rejectUser_notFound_throws()`: `assertThrows(ResponseStatusException.class, ...)``assertThrows(IllegalArgumentException.class, ...)`
- [ ] Import für `ResponseStatusException` kann entfernt werden (prüfen ob noch used)
- [ ] Alle Tests grün
**Details:**
```java
@Test
void rejectUser_notPending_throws() {
testUser.setStatus(RegistrationStatus.APPROVED);
when(userRepository.findById(testUser.getId())).thenReturn(Optional.of(testUser));
assertThrows(IllegalStateException.class, () -> adminIamService.rejectUser(testUser.getId()));
verify(userRepository, never()).save(any());
}
@Test
void rejectUser_notFound_throws() {
UUID unknownId = UUID.randomUUID();
when(userRepository.findById(unknownId)).thenReturn(Optional.empty());
assertThrows(IllegalArgumentException.class, () -> adminIamService.rejectUser(unknownId));
}
```
---
## T11: GlobalExceptionHandlerTest — neue Tests schreiben
**Beschreibung:** Erstelle Unit-Tests für den GlobalExceptionHandler.
**Dateien:**
- `eeg_backend/src/test/java/at/mueller/eeg/backend/common/exception/GlobalExceptionHandlerTest.java` (neu)
**Akzeptanz:**
- [ ] Test für `MethodArgumentNotValidException` → 400 mit Fehlermeldungen
- [ ] Test für `AtNumberAlreadyExistsException` → 409 mit ex.getMessage()
- [ ] Test für `IllegalArgumentException` → 400
- [ ] Test für `IllegalStateException` → 409
- [ ] Test für `AccessDeniedException` → 403
- [ ] Test für `BadCredentialsException` → 401
- [ ] Test für `DisabledException` → 403
- [ ] Test für `Exception` (catch-all) → 500
- [ ] Alle Tests grün
**Details:**
```java
@ExtendWith(MockitoExtension.class)
class GlobalExceptionHandlerTest {
private final GlobalExceptionHandler handler = new GlobalExceptionHandler();
@Test
void handleValidation_returns400WithFieldErrors() {
// Mock MethodArgumentNotValidException mit BindingResult
// Prüfe: 400, ErrorResponse mit Feldfehlern
}
@Test
void handleAtNumberAlreadyExists_returns409WithMessage() {
AtNumberAlreadyExistsException ex = new AtNumberAlreadyExistsException("AT-Nummer 'AT123' existiert bereits.");
ResponseEntity<ErrorResponse> response = handler.handleAtNumberAlreadyExists(ex);
assertEquals(409, response.getStatusCode().value());
assertEquals("AT-Nummer 'AT123' existiert bereits.", response.getBody().message());
}
// ... weitere Tests
}
```
---
## T12: NotificationServiceTest — Ownership-Check testen
**Beschreibung:** Aktualisiere den Test für `markAsRead` und füge Ownership-Check-Tests hinzu.
**Dateien:**
- `eeg_backend/src/test/java/at/mueller/eeg/backend/common/service/NotificationServiceTest.java`
**Akzeptanz:**
- [ ] `markAsRead_setsReadToTrue()` aktualisiert: Aufruf mit `(id, userId)`
- [ ] Neuer Test: `markAsRead_throwsIfNotOwner()` — Ownership-Check
- [ ] Neuer Test: `markAsRead_throwsIfNotFound()` — nicht existierende ID
- [ ] Alle Tests grün
**Details:**
```java
@Test
void markAsRead_setsReadToTrue() {
when(notificationRepository.findById(testNotification.getId()))
.thenReturn(Optional.of(testNotification));
when(notificationRepository.save(any())).thenReturn(testNotification);
notificationService.markAsRead(testNotification.getId(), testUserId);
assertTrue(testNotification.isRead());
verify(notificationRepository).save(testNotification);
}
@Test
void markAsRead_throwsIfNotOwner() {
UUID otherUserId = UUID.randomUUID();
when(notificationRepository.findById(testNotification.getId()))
.thenReturn(Optional.of(testNotification));
assertThrows(AccessDeniedException.class,
() -> notificationService.markAsRead(testNotification.getId(), otherUserId));
verify(notificationRepository, never()).save(any());
}
@Test
void markAsRead_throwsIfNotFound() {
UUID unknownId = UUID.randomUUID();
when(notificationRepository.findById(unknownId)).thenReturn(Optional.empty());
assertThrows(IllegalArgumentException.class,
() -> notificationService.markAsRead(unknownId, testUserId));
}
```
---
## T13: TariffServiceTest — Tests nach Refactoring prüfen
**Beschreibung:** Führe alle 25 TariffServiceTests aus und prüfe, ob sie nach dem Refactoring (T7) weiterhin grün sind.
**Dateien:**
- `eeg_backend/src/test/java/at/mueller/eeg/backend/tariff/service/TariffServiceTest.java`
**Akzeptanz:**
- [ ] Alle 25 Tests grün
- [ ] Keine neuen Tests nötig (Refactoring ohne Verhaltensänderung)
---
## T14: Alle Backend-Tests ausführen
**Beschreibung:** Führe die gesamte Backend-Test-Suite aus, um Regressionen auszuschließen.
**Befehl:**
```bash
.\mvnw test -pl eeg_backend
```
**Akzeptanz:**
- [ ] Alle Tests grün
- [ ] Keine Compile-Fehler