@@ -0,0 +1,71 @@
|
||||
package org.raddatz.familienarchiv.controller;
|
||||
|
|
||||
|
||||
import lombok.RequiredArgsConstructor;
|
||||
import org.raddatz.familienarchiv.dto.NotificationPreferenceDTO;
|
||||
import org.raddatz.familienarchiv.model.AppUser;
|
||||
import org.raddatz.familienarchiv.model.Notification;
|
||||
import org.raddatz.familienarchiv.service.NotificationService;
|
||||
import org.raddatz.familienarchiv.service.UserService;
|
||||
import org.springframework.data.domain.Page;
|
||||
import org.springframework.data.domain.PageRequest;
|
||||
import org.springframework.data.domain.Sort;
|
||||
import org.springframework.http.HttpStatus;
|
||||
import org.springframework.security.core.Authentication;
|
||||
import org.springframework.web.bind.annotation.*;
|
||||
|
||||
import java.util.UUID;
|
||||
|
||||
@RestController
|
||||
@RequiredArgsConstructor
|
||||
public class NotificationController {
|
||||
|
||||
private final NotificationService notificationService;
|
||||
private final UserService userService;
|
||||
|
||||
@GetMapping("/api/notifications")
|
||||
public Page<Notification> getNotifications(
|
||||
|
marcel
commented
Avoid returning the
Create a Return **Avoid returning the `Notification` entity directly from the controller.**
`Notification` has `@ManyToOne(fetch = FetchType.LAZY)` on `recipient`. Jackson will trigger lazy loading for every notification in the page to serialize the `AppUser` — that's another N+1 in the serialization path. Worse, `AppUser` contains fields (password hash, groups) that should never leave the backend.
Create a `NotificationDTO` record:
```java
public record NotificationDTO(
UUID id,
NotificationType type,
UUID documentId,
UUID referenceId,
boolean read,
LocalDateTime createdAt,
String actorName
) {}
```
Return `Page<NotificationDTO>` from `getNotifications` and `NotificationDTO` from `markOneRead`. The service maps `Notification → NotificationDTO` before returning. This sidesteps both the N+1 and the data-leakage risk.
|
||||
@RequestParam(defaultValue = "0") int page,
|
||||
@RequestParam(defaultValue = "10") int size,
|
||||
Authentication authentication) {
|
||||
AppUser user = resolveUser(authentication);
|
||||
PageRequest pageable = PageRequest.of(page, size, Sort.by("createdAt").descending());
|
||||
return notificationService.getNotifications(user.getId(), pageable);
|
||||
}
|
||||
|
||||
@PostMapping("/api/notifications/read-all")
|
||||
@ResponseStatus(HttpStatus.NO_CONTENT)
|
||||
public void markAllRead(Authentication authentication) {
|
||||
AppUser user = resolveUser(authentication);
|
||||
notificationService.markAllRead(user.getId());
|
||||
}
|
||||
|
||||
@PatchMapping("/api/notifications/{id}/read")
|
||||
public Notification markOneRead(
|
||||
@PathVariable UUID id,
|
||||
Authentication authentication) {
|
||||
AppUser user = resolveUser(authentication);
|
||||
return notificationService.markRead(id, user.getId());
|
||||
}
|
||||
|
||||
@GetMapping("/api/users/me/notification-preferences")
|
||||
public NotificationPreferenceDTO getPreferences(Authentication authentication) {
|
||||
AppUser user = resolveUser(authentication);
|
||||
return new NotificationPreferenceDTO(user.isNotifyOnReply(), user.isNotifyOnMention());
|
||||
}
|
||||
|
||||
@PutMapping("/api/users/me/notification-preferences")
|
||||
public NotificationPreferenceDTO updatePreferences(
|
||||
@RequestBody NotificationPreferenceDTO dto,
|
||||
Authentication authentication) {
|
||||
AppUser user = resolveUser(authentication);
|
||||
AppUser updated = notificationService.updatePreferences(
|
||||
user.getId(), dto.notifyOnReply(), dto.notifyOnMention());
|
||||
return new NotificationPreferenceDTO(updated.isNotifyOnReply(), updated.isNotifyOnMention());
|
||||
}
|
||||
|
||||
// ─── private helpers ──────────────────────────────────────────────────────
|
||||
|
||||
private AppUser resolveUser(Authentication authentication) {
|
||||
return userService.findByUsername(authentication.getName());
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,3 @@
|
||||
package org.raddatz.familienarchiv.dto;
|
||||
|
||||
public record NotificationPreferenceDTO(boolean notifyOnReply, boolean notifyOnMention) {}
|
||||
@@ -50,6 +50,10 @@ public enum ErrorCode {
|
||||
/** The comment with the given ID does not exist. 404 */
|
||||
COMMENT_NOT_FOUND,
|
||||
|
||||
// --- Notifications ---
|
||||
/** The notification with the given ID does not exist. 404 */
|
||||
NOTIFICATION_NOT_FOUND,
|
||||
|
||||
// --- Generic ---
|
||||
/** Request validation failed (missing or malformed fields). 400 */
|
||||
VALIDATION_ERROR,
|
||||
|
||||
@@ -51,6 +51,16 @@ public class AppUser {
|
||||
@Schema(requiredMode = Schema.RequiredMode.REQUIRED)
|
||||
private boolean enabled = true; // Um User zu sperren ohne sie zu löschen
|
||||
|
||||
@Column(nullable = false)
|
||||
@Builder.Default
|
||||
@Schema(requiredMode = Schema.RequiredMode.REQUIRED)
|
||||
private boolean notifyOnReply = false;
|
||||
|
||||
@Column(nullable = false)
|
||||
@Builder.Default
|
||||
@Schema(requiredMode = Schema.RequiredMode.REQUIRED)
|
||||
private boolean notifyOnMention = false;
|
||||
|
||||
// Ein User kann in mehreren Gruppen sein
|
||||
@ManyToMany(fetch = FetchType.EAGER)
|
||||
@JoinTable(name = "users_groups", joinColumns = @JoinColumn(name = "user_id"), inverseJoinColumns = @JoinColumn(name = "group_id"))
|
||||
|
||||
@@ -0,0 +1,53 @@
|
||||
package org.raddatz.familienarchiv.model;
|
||||
|
||||
import io.swagger.v3.oas.annotations.media.Schema;
|
||||
import jakarta.persistence.*;
|
||||
import lombok.*;
|
||||
import org.hibernate.annotations.CreationTimestamp;
|
||||
|
||||
import java.time.LocalDateTime;
|
||||
import java.util.UUID;
|
||||
|
||||
@Entity
|
||||
@Table(name = "notifications")
|
||||
@Data
|
||||
@NoArgsConstructor
|
||||
@AllArgsConstructor
|
||||
@Builder
|
||||
public class Notification {
|
||||
|
||||
@Id
|
||||
@GeneratedValue(strategy = GenerationType.UUID)
|
||||
@Schema(requiredMode = Schema.RequiredMode.REQUIRED)
|
||||
private UUID id;
|
||||
|
||||
@ManyToOne(fetch = FetchType.LAZY)
|
||||
@JoinColumn(name = "recipient_id", nullable = false)
|
||||
@Schema(requiredMode = Schema.RequiredMode.REQUIRED)
|
||||
private AppUser recipient;
|
||||
|
||||
@Enumerated(EnumType.STRING)
|
||||
@Column(nullable = false)
|
||||
@Schema(requiredMode = Schema.RequiredMode.REQUIRED)
|
||||
private NotificationType type;
|
||||
|
||||
@Column(name = "document_id")
|
||||
private UUID documentId;
|
||||
|
||||
@Column(name = "reference_id")
|
||||
private UUID referenceId;
|
||||
|
||||
@Column(nullable = false)
|
||||
@Builder.Default
|
||||
@Schema(requiredMode = Schema.RequiredMode.REQUIRED)
|
||||
private boolean read = false;
|
||||
|
||||
@CreationTimestamp
|
||||
@Schema(requiredMode = Schema.RequiredMode.REQUIRED)
|
||||
private LocalDateTime createdAt;
|
||||
|
||||
// Populated by NotificationService before serialization — not persisted.
|
||||
@Transient
|
||||
@Schema(requiredMode = Schema.RequiredMode.REQUIRED)
|
||||
private String actorName;
|
||||
}
|
||||
|
marcel
commented
Bug:
Fix in Fix in For **Bug: `actorName` is never set — will always be `null` in every API response.**
`actorName` is `@Transient` (not persisted), so it must be populated before serialization. But neither `notifyReply` nor `notifyMentions` in `NotificationService` ever calls `.actorName(...)` on the builder. The frontend renders `notification.actorName` directly in the bell dropdown — users will see an empty string where the sender's name should be.
Fix in `notifyReply`:
```java
.actorName(reply.getAuthorName())
```
Fix in `notifyMentions`:
```java
.actorName(comment.getAuthorName())
```
For `getNotifications()` (the paginated fetch), stored notifications already lack `actorName`, so you'll also need to re-hydrate it from the comment or store it as a plain column — storing it as a `VARCHAR` column in the migration is the simplest option, since `authorName` on `DocumentComment` is already a denormalized string.
|
||||
@@ -0,0 +1,6 @@
|
||||
package org.raddatz.familienarchiv.model;
|
||||
|
||||
public enum NotificationType {
|
||||
REPLY,
|
||||
MENTION
|
||||
}
|
||||
@@ -0,0 +1,25 @@
|
||||
package org.raddatz.familienarchiv.repository;
|
||||
|
marcel
commented
ℹ️ INFO — Unused method: The non-paginated Fix: Remove the method. ℹ️ **INFO — Unused method: `findByRecipientIdOrderByCreatedAtDesc`**
The non-paginated `List<Notification> findByRecipientIdOrderByCreatedAtDesc(UUID)` is declared but never called — the paginated `findByRecipientId(UUID, Pageable)` is used exclusively. Dead code in repositories adds noise and risks someone accidentally calling the unbounded query on a user with thousands of notifications.
**Fix:** Remove the method.
|
||||
|
||||
import org.raddatz.familienarchiv.model.Notification;
|
||||
import org.springframework.data.domain.Page;
|
||||
import org.springframework.data.domain.Pageable;
|
||||
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 java.util.List;
|
||||
import java.util.UUID;
|
||||
|
||||
public interface NotificationRepository extends JpaRepository<Notification, UUID> {
|
||||
|
||||
Page<Notification> findByRecipientIdOrderByCreatedAtDesc(UUID recipientId, Pageable pageable);
|
||||
|
||||
long countByRecipientIdAndReadFalse(UUID recipientId);
|
||||
|
||||
@Modifying
|
||||
@Query("UPDATE Notification n SET n.read = true WHERE n.recipient.id = :userId")
|
||||
void markAllReadByRecipientId(@Param("userId") UUID userId);
|
||||
|
||||
List<Notification> findByRecipientIdOrderByCreatedAtDesc(UUID recipientId);
|
||||
}
|
||||
|
marcel
commented
Dead code: the You have two methods with the same name:
Delete the **Dead code: the `List<Notification>` overload is never called.**
You have two methods with the same name:
- `Page<Notification> findByRecipientIdOrderByCreatedAtDesc(UUID recipientId, Pageable pageable)` — used by the service
- `List<Notification> findByRecipientIdOrderByCreatedAtDesc(UUID recipientId)` — unused
Delete the `List` variant. If you ever need it, add it back then.
|
||||
@@ -0,0 +1,187 @@
|
||||
package org.raddatz.familienarchiv.service;
|
||||
|
marcel
commented
🚨 BLOCKER — Fragile mixed injection + ReflectionTestUtils hack The class uses Fix: Inject 🚨 **BLOCKER — Fragile mixed injection + ReflectionTestUtils hack**
The class uses `@RequiredArgsConstructor` but `mailSender` is injected via `@Autowired(required = false)` — a mixed injection approach. The test is forced to use `ReflectionTestUtils.setField()` to inject the mock. This is a maintenance trap: if `mailSender` is ever renamed, the reflection call silently breaks at runtime with no compile-time error — the email path stops being tested without any CI failure.
**Fix:** Inject `Optional<JavaMailSender>` as a constructor parameter (works with Lombok `@RequiredArgsConstructor`) or use `@ConditionalOnBean(JavaMailSender.class)` on a separate email-sending component. Either approach is injection-safe and doesn't require reflection in tests.
|
||||
|
||||
|
marcel
commented
⚠️ MAJOR — Notification failure can silently roll back the parent comment
A user's comment should never disappear because a notification couldn't be persisted. Fix: Annotate both methods with ⚠️ **MAJOR — Notification failure can silently roll back the parent comment**
`notifyReply()` and `notifyMentions()` are `@Transactional` and are called from within the already-`@Transactional` `replyToComment()` / `postComment()` in `CommentService`. They join the outer transaction. If a notification save fails (DB constraint, unexpected null), it rolls back the entire outer transaction — the comment itself is lost.
A user's comment should never disappear because a notification couldn't be persisted.
**Fix:** Annotate both methods with `@Transactional(propagation = Propagation.REQUIRES_NEW)` to isolate notification failures, or use `@Async` to fire them outside the comment transaction entirely.
|
||||
import lombok.RequiredArgsConstructor;
|
||||
import lombok.extern.slf4j.Slf4j;
|
||||
import org.raddatz.familienarchiv.exception.DomainException;
|
||||
import org.raddatz.familienarchiv.exception.ErrorCode;
|
||||
import org.raddatz.familienarchiv.model.AppUser;
|
||||
import org.raddatz.familienarchiv.model.DocumentComment;
|
||||
import org.raddatz.familienarchiv.model.Notification;
|
||||
import org.raddatz.familienarchiv.model.NotificationType;
|
||||
import org.raddatz.familienarchiv.repository.AppUserRepository;
|
||||
import org.raddatz.familienarchiv.repository.CommentRepository;
|
||||
import org.raddatz.familienarchiv.repository.NotificationRepository;
|
||||
import org.springframework.beans.factory.annotation.Autowired;
|
||||
import org.springframework.beans.factory.annotation.Value;
|
||||
import org.springframework.data.domain.Page;
|
||||
import org.springframework.data.domain.Pageable;
|
||||
import org.springframework.mail.MailException;
|
||||
import org.springframework.mail.SimpleMailMessage;
|
||||
import org.springframework.mail.javamail.JavaMailSender;
|
||||
import org.springframework.stereotype.Service;
|
||||
import org.springframework.transaction.annotation.Transactional;
|
||||
|
||||
import java.util.LinkedHashSet;
|
||||
import java.util.List;
|
||||
import java.util.Optional;
|
||||
import java.util.Set;
|
||||
import java.util.UUID;
|
||||
|
||||
@Service
|
||||
@RequiredArgsConstructor
|
||||
@Slf4j
|
||||
public class NotificationService {
|
||||
|
||||
private final NotificationRepository notificationRepository;
|
||||
private final CommentRepository commentRepository;
|
||||
private final AppUserRepository userRepository;
|
||||
|
||||
@Autowired(required = false)
|
||||
private JavaMailSender mailSender;
|
||||
|
||||
@Value("${app.mail.from:noreply@familienarchiv.local}")
|
||||
private String mailFrom;
|
||||
|
||||
@Value("${app.base-url:http://localhost:3000}")
|
||||
private String baseUrl;
|
||||
|
||||
/**
|
||||
* Creates REPLY notifications for all participants in the thread that the given reply belongs to,
|
||||
* excluding the replier themselves.
|
||||
*/
|
||||
@Transactional
|
||||
public void notifyReply(DocumentComment reply, DocumentComment root) {
|
||||
Set<UUID> participantIds = collectParticipantIds(root);
|
||||
participantIds.remove(reply.getAuthorId());
|
||||
|
||||
for (UUID participantId : participantIds) {
|
||||
Optional<AppUser> recipientOpt = userRepository.findById(participantId);
|
||||
|
marcel
commented
N+1: For every participant ID you're issuing a separate SELECT. If a thread has 10 participants that's 10 round-trips to the database before a single notification is written. Replace the loop body with a single bulk fetch: Same pattern applies to **N+1: `findById` inside a loop.**
For every participant ID you're issuing a separate SELECT. If a thread has 10 participants that's 10 round-trips to the database before a single notification is written.
Replace the loop body with a single bulk fetch:
```java
public void notifyReply(DocumentComment reply, DocumentComment root) {
Set<UUID> participantIds = collectParticipantIds(root);
participantIds.remove(reply.getAuthorId());
if (participantIds.isEmpty()) return;
List<AppUser> recipients = userRepository.findAllById(participantIds);
for (AppUser recipient : recipients) {
Notification notification = Notification.builder()
.recipient(recipient)
.type(NotificationType.REPLY)
.documentId(reply.getDocumentId())
.referenceId(reply.getId())
.actorName(reply.getAuthorName())
.build();
notificationRepository.save(notification);
if (recipient.isNotifyOnReply()) {
sendNotificationEmail(recipient, reply, NotificationType.REPLY);
}
}
}
```
Same pattern applies to `notifyMentions` below.
|
||||
if (recipientOpt.isEmpty()) continue;
|
||||
|
||||
AppUser recipient = recipientOpt.get();
|
||||
Notification notification = Notification.builder()
|
||||
.recipient(recipient)
|
||||
.type(NotificationType.REPLY)
|
||||
.documentId(reply.getDocumentId())
|
||||
.referenceId(reply.getId())
|
||||
.build();
|
||||
notificationRepository.save(notification);
|
||||
|
||||
if (recipient.isNotifyOnReply()) {
|
||||
sendNotificationEmail(recipient, reply, NotificationType.REPLY);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Creates MENTION notifications for each mentioned user.
|
||||
*/
|
||||
@Transactional
|
||||
public void notifyMentions(List<UUID> mentionedUserIds, DocumentComment comment) {
|
||||
for (UUID mentionedUserId : mentionedUserIds) {
|
||||
Optional<AppUser> recipientOpt = userRepository.findById(mentionedUserId);
|
||||
|
marcel
commented
N+1: same issue as For
**N+1: same issue as `notifyReply` — `findById` in a loop.**
For `notifyMentions`:
```java
List<AppUser> recipients = userRepository.findAllById(mentionedUserIds);
for (AppUser recipient : recipients) { ... }
```
`findAllById` emits a single `SELECT … WHERE id IN (…)` instead of one query per user.
|
||||
if (recipientOpt.isEmpty()) continue;
|
||||
|
||||
AppUser recipient = recipientOpt.get();
|
||||
Notification notification = Notification.builder()
|
||||
.recipient(recipient)
|
||||
.type(NotificationType.MENTION)
|
||||
.documentId(comment.getDocumentId())
|
||||
.referenceId(comment.getId())
|
||||
.build();
|
||||
notificationRepository.save(notification);
|
||||
|
||||
if (recipient.isNotifyOnMention()) {
|
||||
sendNotificationEmail(recipient, comment, NotificationType.MENTION);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
public Page<Notification> getNotifications(UUID userId, Pageable pageable) {
|
||||
return notificationRepository.findByRecipientIdOrderByCreatedAtDesc(userId, pageable);
|
||||
}
|
||||
|
||||
public long countUnread(UUID userId) {
|
||||
return notificationRepository.countByRecipientIdAndReadFalse(userId);
|
||||
}
|
||||
|
||||
@Transactional
|
||||
public void markAllRead(UUID userId) {
|
||||
notificationRepository.markAllReadByRecipientId(userId);
|
||||
}
|
||||
|
||||
@Transactional
|
||||
public Notification markRead(UUID notificationId, UUID userId) {
|
||||
Notification notification = notificationRepository.findById(notificationId)
|
||||
.orElseThrow(() -> DomainException.notFound(
|
||||
ErrorCode.NOTIFICATION_NOT_FOUND, "Notification not found: " + notificationId));
|
||||
if (!notification.getRecipient().getId().equals(userId)) {
|
||||
throw DomainException.forbidden("Notification belongs to a different user");
|
||||
}
|
||||
notification.setRead(true);
|
||||
return notificationRepository.save(notification);
|
||||
}
|
||||
|
||||
@Transactional
|
||||
public AppUser updatePreferences(UUID userId, boolean notifyOnReply, boolean notifyOnMention) {
|
||||
AppUser user = userRepository.findById(userId)
|
||||
.orElseThrow(() -> DomainException.notFound(ErrorCode.USER_NOT_FOUND, "User not found: " + userId));
|
||||
user.setNotifyOnReply(notifyOnReply);
|
||||
user.setNotifyOnMention(notifyOnMention);
|
||||
return userRepository.save(user);
|
||||
}
|
||||
|
||||
// ─── private helpers ──────────────────────────────────────────────────────
|
||||
|
||||
private Set<UUID> collectParticipantIds(DocumentComment root) {
|
||||
Set<UUID> ids = new LinkedHashSet<>();
|
||||
if (root.getAuthorId() != null) ids.add(root.getAuthorId());
|
||||
|
||||
commentRepository.findByParentId(root.getId())
|
||||
.forEach(reply -> {
|
||||
if (reply.getAuthorId() != null) ids.add(reply.getAuthorId());
|
||||
});
|
||||
return ids;
|
||||
}
|
||||
|
||||
private void buildCommentPath(DocumentComment comment, StringBuilder sb) {
|
||||
sb.append("?commentId=").append(comment.getId());
|
||||
if (comment.getAnnotationId() != null) {
|
||||
sb.append("&annotationId=").append(comment.getAnnotationId());
|
||||
}
|
||||
}
|
||||
|
||||
private void sendNotificationEmail(AppUser recipient, DocumentComment comment, NotificationType type) {
|
||||
if (mailSender == null) {
|
||||
log.warn("Mail sender not configured — skipping notification email to {}", recipient.getEmail());
|
||||
return;
|
||||
}
|
||||
if (recipient.getEmail() == null || recipient.getEmail().isBlank()) return;
|
||||
|
||||
StringBuilder path = new StringBuilder("/documents/").append(comment.getDocumentId());
|
||||
buildCommentPath(comment, path);
|
||||
String link = baseUrl + path;
|
||||
|
||||
String subject = type == NotificationType.REPLY
|
||||
? "Neue Antwort auf deinen Kommentar — Familienarchiv"
|
||||
: "Du wurdest in einem Kommentar erwähnt — Familienarchiv";
|
||||
|
||||
String body = type == NotificationType.REPLY
|
||||
? "Hallo,\n\njemand hat auf einen Kommentar geantwortet, an dem du beteiligt warst.\n\n"
|
||||
+ "Zum Kommentar:\n" + link + "\n\nDein Familienarchiv-Team"
|
||||
: "Hallo,\n\njemand hat dich in einem Kommentar erwähnt.\n\n"
|
||||
+ "Zum Kommentar:\n" + link + "\n\nDein Familienarchiv-Team";
|
||||
|
||||
SimpleMailMessage message = new SimpleMailMessage();
|
||||
message.setFrom(mailFrom);
|
||||
message.setTo(recipient.getEmail());
|
||||
message.setSubject(subject);
|
||||
message.setText(body);
|
||||
|
||||
try {
|
||||
mailSender.send(message);
|
||||
} catch (MailException e) {
|
||||
log.error("Failed to send notification email to {}: {}", recipient.getEmail(), e.getMessage());
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,162 @@
|
||||
package org.raddatz.familienarchiv.controller;
|
||||
|
marcel
commented
⚠️ MAJOR — Every other endpoint in this controller has an unauthenticated (401) test. The Fix: Add ⚠️ **MAJOR — `PATCH /api/notifications/{id}/read` missing 401 test**
Every other endpoint in this controller has an unauthenticated (401) test. The `markOneRead` endpoint is the only one missing it. This inconsistency means a security regression specifically on this endpoint would not be caught by CI.
**Fix:** Add `markOneRead_returns401_whenUnauthenticated()` mirroring the pattern already used for `getNotifications_returns401_whenUnauthenticated`.
|
||||
|
||||
import org.junit.jupiter.api.Test;
|
||||
import org.raddatz.familienarchiv.config.SecurityConfig;
|
||||
import org.raddatz.familienarchiv.model.AppUser;
|
||||
import org.raddatz.familienarchiv.model.Notification;
|
||||
import org.raddatz.familienarchiv.model.NotificationType;
|
||||
import org.raddatz.familienarchiv.security.PermissionAspect;
|
||||
import org.raddatz.familienarchiv.service.CustomUserDetailsService;
|
||||
import org.raddatz.familienarchiv.service.NotificationService;
|
||||
import org.raddatz.familienarchiv.service.UserService;
|
||||
import org.springframework.beans.factory.annotation.Autowired;
|
||||
import org.springframework.boot.autoconfigure.aop.AopAutoConfiguration;
|
||||
import org.springframework.boot.webmvc.test.autoconfigure.WebMvcTest;
|
||||
import org.springframework.context.annotation.Import;
|
||||
import org.springframework.data.domain.PageImpl;
|
||||
import org.springframework.data.domain.PageRequest;
|
||||
import org.springframework.http.MediaType;
|
||||
import org.springframework.security.test.context.support.WithMockUser;
|
||||
import org.springframework.test.context.bean.override.mockito.MockitoBean;
|
||||
import org.springframework.test.web.servlet.MockMvc;
|
||||
|
||||
import java.util.List;
|
||||
import java.util.UUID;
|
||||
|
||||
import static org.mockito.ArgumentMatchers.any;
|
||||
import static org.mockito.ArgumentMatchers.eq;
|
||||
import static org.mockito.Mockito.verify;
|
||||
import static org.mockito.Mockito.when;
|
||||
import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.*;
|
||||
import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.*;
|
||||
|
||||
@WebMvcTest(NotificationController.class)
|
||||
@Import({SecurityConfig.class, PermissionAspect.class, AopAutoConfiguration.class})
|
||||
class NotificationControllerTest {
|
||||
|
||||
@Autowired MockMvc mockMvc;
|
||||
|
||||
@MockitoBean NotificationService notificationService;
|
||||
@MockitoBean UserService userService;
|
||||
@MockitoBean CustomUserDetailsService customUserDetailsService;
|
||||
|
||||
private static final UUID USER_ID = UUID.randomUUID();
|
||||
|
||||
// ─── GET /api/notifications ───────────────────────────────────────────────
|
||||
|
||||
@Test
|
||||
void getNotifications_returns401_whenUnauthenticated() throws Exception {
|
||||
mockMvc.perform(get("/api/notifications"))
|
||||
.andExpect(status().isUnauthorized());
|
||||
}
|
||||
|
||||
@Test
|
||||
@WithMockUser(username = "testuser")
|
||||
void getNotifications_returns200WithList_whenAuthenticated() throws Exception {
|
||||
AppUser user = AppUser.builder().id(USER_ID).username("testuser").build();
|
||||
Notification n = Notification.builder()
|
||||
.id(UUID.randomUUID()).recipient(user)
|
||||
.type(NotificationType.REPLY).read(false).build();
|
||||
|
||||
when(userService.findByUsername("testuser")).thenReturn(user);
|
||||
when(notificationService.getNotifications(eq(USER_ID), any()))
|
||||
.thenReturn(new PageImpl<>(List.of(n), PageRequest.of(0, 10), 1));
|
||||
|
||||
mockMvc.perform(get("/api/notifications"))
|
||||
.andExpect(status().isOk())
|
||||
.andExpect(jsonPath("$.content").isArray());
|
||||
}
|
||||
|
||||
@Test
|
||||
@WithMockUser(username = "testuser")
|
||||
void getNotifications_returnsOnlyCurrentUsersNotifications() throws Exception {
|
||||
AppUser user = AppUser.builder().id(USER_ID).username("testuser").build();
|
||||
when(userService.findByUsername("testuser")).thenReturn(user);
|
||||
when(notificationService.getNotifications(eq(USER_ID), any()))
|
||||
.thenReturn(new PageImpl<>(List.of()));
|
||||
|
||||
mockMvc.perform(get("/api/notifications"))
|
||||
.andExpect(status().isOk());
|
||||
|
||||
verify(notificationService).getNotifications(eq(USER_ID), any());
|
||||
}
|
||||
|
||||
// ─── POST /api/notifications/read-all ────────────────────────────────────
|
||||
|
||||
@Test
|
||||
void markAllRead_returns401_whenUnauthenticated() throws Exception {
|
||||
mockMvc.perform(post("/api/notifications/read-all"))
|
||||
.andExpect(status().isUnauthorized());
|
||||
}
|
||||
|
||||
@Test
|
||||
@WithMockUser(username = "testuser")
|
||||
void markAllRead_returns204_whenAuthenticated() throws Exception {
|
||||
AppUser user = AppUser.builder().id(USER_ID).username("testuser").build();
|
||||
when(userService.findByUsername("testuser")).thenReturn(user);
|
||||
|
||||
mockMvc.perform(post("/api/notifications/read-all"))
|
||||
.andExpect(status().isNoContent());
|
||||
|
||||
verify(notificationService).markAllRead(USER_ID);
|
||||
}
|
||||
|
||||
// ─── PATCH /api/notifications/{id}/read ──────────────────────────────────
|
||||
|
||||
@Test
|
||||
@WithMockUser(username = "testuser")
|
||||
void markOneRead_returns403_whenNotificationBelongsToDifferentUser() throws Exception {
|
||||
AppUser user = AppUser.builder().id(USER_ID).username("testuser").build();
|
||||
UUID notifId = UUID.randomUUID();
|
||||
|
||||
when(userService.findByUsername("testuser")).thenReturn(user);
|
||||
org.mockito.Mockito.doThrow(
|
||||
org.raddatz.familienarchiv.exception.DomainException.forbidden("not yours"))
|
||||
.when(notificationService).markRead(notifId, USER_ID);
|
||||
|
||||
mockMvc.perform(patch("/api/notifications/" + notifId + "/read"))
|
||||
.andExpect(status().isForbidden());
|
||||
}
|
||||
|
||||
// ─── GET /api/users/me/notification-preferences ──────────────────────────
|
||||
|
||||
@Test
|
||||
void getPreferences_returns401_whenUnauthenticated() throws Exception {
|
||||
mockMvc.perform(get("/api/users/me/notification-preferences"))
|
||||
.andExpect(status().isUnauthorized());
|
||||
}
|
||||
|
||||
@Test
|
||||
@WithMockUser(username = "testuser")
|
||||
void getPreferences_returnsCurrentPreferences() throws Exception {
|
||||
AppUser user = AppUser.builder().id(USER_ID).username("testuser")
|
||||
.notifyOnReply(true).notifyOnMention(false).build();
|
||||
when(userService.findByUsername("testuser")).thenReturn(user);
|
||||
|
||||
mockMvc.perform(get("/api/users/me/notification-preferences"))
|
||||
.andExpect(status().isOk())
|
||||
.andExpect(jsonPath("$.notifyOnReply").value(true))
|
||||
.andExpect(jsonPath("$.notifyOnMention").value(false));
|
||||
}
|
||||
|
||||
// ─── PUT /api/users/me/notification-preferences ──────────────────────────
|
||||
|
||||
@Test
|
||||
@WithMockUser(username = "testuser")
|
||||
void updatePreferences_persistsBothBooleans() throws Exception {
|
||||
AppUser user = AppUser.builder().id(USER_ID).username("testuser")
|
||||
.notifyOnReply(false).notifyOnMention(false).build();
|
||||
when(userService.findByUsername("testuser")).thenReturn(user);
|
||||
|
||||
AppUser updated = AppUser.builder().id(USER_ID).username("testuser")
|
||||
.notifyOnReply(true).notifyOnMention(true).build();
|
||||
when(notificationService.updatePreferences(USER_ID, true, true)).thenReturn(updated);
|
||||
|
||||
mockMvc.perform(put("/api/users/me/notification-preferences")
|
||||
.contentType(MediaType.APPLICATION_JSON)
|
||||
.content("{\"notifyOnReply\":true,\"notifyOnMention\":true}"))
|
||||
.andExpect(status().isOk())
|
||||
.andExpect(jsonPath("$.notifyOnReply").value(true))
|
||||
.andExpect(jsonPath("$.notifyOnMention").value(true));
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,201 @@
|
||||
package org.raddatz.familienarchiv.service;
|
||||
|
marcel
commented
⚠️ MAJOR — Several service method paths are untested Missing test cases:
These are not exotic edge cases — they are production paths that the frontend already exercises. ⚠️ **MAJOR — Several service method paths are untested**
Missing test cases:
- `markRead_throwsNotFound_whenNotificationDoesNotExist` — the `NOTIFICATION_NOT_FOUND` code path is real and user-facing (stale ID from frontend). No test covers it.
- `markAllRead_delegatesToRepository` — `markAllRead()` is tested end-to-end via the controller but never in isolation at the service layer.
- `countUnread_delegatesToRepository` — same gap.
These are not exotic edge cases — they are production paths that the frontend already exercises.
|
||||
|
||||
import org.junit.jupiter.api.BeforeEach;
|
||||
import org.junit.jupiter.api.Test;
|
||||
import org.junit.jupiter.api.extension.ExtendWith;
|
||||
import org.mockito.ArgumentCaptor;
|
||||
import org.mockito.InjectMocks;
|
||||
import org.mockito.Mock;
|
||||
import org.mockito.junit.jupiter.MockitoExtension;
|
||||
import org.springframework.test.util.ReflectionTestUtils;
|
||||
import org.raddatz.familienarchiv.exception.DomainException;
|
||||
import org.raddatz.familienarchiv.model.*;
|
||||
import org.raddatz.familienarchiv.repository.AppUserRepository;
|
||||
import org.raddatz.familienarchiv.repository.CommentRepository;
|
||||
import org.raddatz.familienarchiv.repository.NotificationRepository;
|
||||
import org.springframework.mail.SimpleMailMessage;
|
||||
import org.springframework.mail.javamail.JavaMailSender;
|
||||
|
||||
import java.util.List;
|
||||
import java.util.Optional;
|
||||
import java.util.UUID;
|
||||
|
||||
import static org.assertj.core.api.Assertions.assertThat;
|
||||
import static org.assertj.core.api.Assertions.assertThatThrownBy;
|
||||
import static org.mockito.ArgumentMatchers.any;
|
||||
import static org.mockito.Mockito.*;
|
||||
|
||||
@ExtendWith(MockitoExtension.class)
|
||||
class NotificationServiceTest {
|
||||
|
||||
@Mock NotificationRepository notificationRepository;
|
||||
@Mock CommentRepository commentRepository;
|
||||
@Mock AppUserRepository userRepository;
|
||||
@Mock JavaMailSender mailSender;
|
||||
|
||||
@InjectMocks NotificationService notificationService;
|
||||
|
||||
private AppUser userA;
|
||||
private AppUser userB;
|
||||
private AppUser userC;
|
||||
|
||||
@BeforeEach
|
||||
void setUp() {
|
||||
// mailSender is @Autowired(required=false) — not in the @RequiredArgsConstructor
|
||||
// constructor, so Mockito won't inject it automatically. Inject explicitly.
|
||||
ReflectionTestUtils.setField(notificationService, "mailSender", mailSender);
|
||||
|
||||
userA = AppUser.builder().id(UUID.randomUUID()).username("userA")
|
||||
.firstName("Anna").lastName("Smith").email("a@test.com")
|
||||
.notifyOnReply(false).notifyOnMention(false).build();
|
||||
userB = AppUser.builder().id(UUID.randomUUID()).username("userB")
|
||||
.firstName("Bob").lastName("Jones").email("b@test.com")
|
||||
.notifyOnReply(false).notifyOnMention(false).build();
|
||||
userC = AppUser.builder().id(UUID.randomUUID()).username("userC")
|
||||
.firstName("Clara").lastName("Doe").email("c@test.com")
|
||||
.notifyOnReply(false).notifyOnMention(false).build();
|
||||
}
|
||||
|
||||
// ─── notifyReply ──────────────────────────────────────────────────────────
|
||||
|
||||
@Test
|
||||
void notifyReply_createsNotificationForThreadParticipant() {
|
||||
DocumentComment root = commentWithAuthor(UUID.randomUUID(), null, userA.getId());
|
||||
DocumentComment existing = commentWithAuthor(UUID.randomUUID(), root.getId(), userB.getId());
|
||||
DocumentComment reply = commentWithAuthor(UUID.randomUUID(), root.getId(), userC.getId());
|
||||
|
||||
when(commentRepository.findByParentId(root.getId())).thenReturn(List.of(existing, reply));
|
||||
when(userRepository.findById(userA.getId())).thenReturn(Optional.of(userA));
|
||||
when(userRepository.findById(userB.getId())).thenReturn(Optional.of(userB));
|
||||
when(notificationRepository.save(any())).thenAnswer(inv -> inv.getArgument(0));
|
||||
|
||||
notificationService.notifyReply(reply, root);
|
||||
|
||||
ArgumentCaptor<Notification> captor = ArgumentCaptor.forClass(Notification.class);
|
||||
verify(notificationRepository, times(2)).save(captor.capture());
|
||||
|
||||
List<Notification> saved = captor.getAllValues();
|
||||
assertThat(saved).extracting(n -> n.getRecipient().getId())
|
||||
.containsExactlyInAnyOrder(userA.getId(), userB.getId());
|
||||
assertThat(saved).allMatch(n -> n.getType() == NotificationType.REPLY);
|
||||
assertThat(saved).allMatch(n -> !n.isRead());
|
||||
}
|
||||
|
||||
@Test
|
||||
void notifyReply_doesNotNotifyTheReplierThemselves() {
|
||||
// userA is both a thread participant and the replier
|
||||
DocumentComment root = commentWithAuthor(UUID.randomUUID(), null, userA.getId());
|
||||
DocumentComment reply = commentWithAuthor(UUID.randomUUID(), root.getId(), userA.getId());
|
||||
|
||||
when(commentRepository.findByParentId(root.getId())).thenReturn(List.of(reply));
|
||||
|
||||
notificationService.notifyReply(reply, root);
|
||||
|
||||
verify(notificationRepository, never()).save(any());
|
||||
}
|
||||
|
||||
@Test
|
||||
void notifyReply_deduplicatesParticipants() {
|
||||
// userB has posted twice in the thread — should get exactly one notification
|
||||
DocumentComment root = commentWithAuthor(UUID.randomUUID(), null, userA.getId());
|
||||
DocumentComment first = commentWithAuthor(UUID.randomUUID(), root.getId(), userB.getId());
|
||||
DocumentComment second = commentWithAuthor(UUID.randomUUID(), root.getId(), userB.getId());
|
||||
DocumentComment reply = commentWithAuthor(UUID.randomUUID(), root.getId(), userC.getId());
|
||||
|
||||
when(commentRepository.findByParentId(root.getId())).thenReturn(List.of(first, second, reply));
|
||||
when(userRepository.findById(userA.getId())).thenReturn(Optional.of(userA));
|
||||
when(userRepository.findById(userB.getId())).thenReturn(Optional.of(userB));
|
||||
when(notificationRepository.save(any())).thenAnswer(inv -> inv.getArgument(0));
|
||||
|
||||
notificationService.notifyReply(reply, root);
|
||||
|
||||
// userA (root) + userB (deduplicated) = 2 notifications, not 3
|
||||
verify(notificationRepository, times(2)).save(any());
|
||||
}
|
||||
|
||||
@Test
|
||||
void notifyReply_sendsEmailOnlyToUsersWithReplyNotificationsEnabled() {
|
||||
userA.setNotifyOnReply(true);
|
||||
userB.setNotifyOnReply(false);
|
||||
|
||||
DocumentComment root = commentWithAuthor(UUID.randomUUID(), null, userA.getId());
|
||||
DocumentComment existing = commentWithAuthor(UUID.randomUUID(), root.getId(), userB.getId());
|
||||
DocumentComment reply = commentWithAuthor(UUID.randomUUID(), root.getId(), userC.getId());
|
||||
|
||||
when(commentRepository.findByParentId(root.getId())).thenReturn(List.of(existing, reply));
|
||||
when(userRepository.findById(userA.getId())).thenReturn(Optional.of(userA));
|
||||
when(userRepository.findById(userB.getId())).thenReturn(Optional.of(userB));
|
||||
when(notificationRepository.save(any())).thenAnswer(inv -> inv.getArgument(0));
|
||||
|
||||
notificationService.notifyReply(reply, root);
|
||||
|
||||
// Only userA has email enabled — one email sent
|
||||
verify(mailSender, times(1)).send(any(SimpleMailMessage.class));
|
||||
}
|
||||
|
||||
// ─── notifyMentions ───────────────────────────────────────────────────────
|
||||
|
||||
@Test
|
||||
void notifyMentions_createsNotificationPerMentionedUser() {
|
||||
DocumentComment comment = commentWithAuthor(UUID.randomUUID(), null, userC.getId());
|
||||
when(userRepository.findById(userA.getId())).thenReturn(Optional.of(userA));
|
||||
when(userRepository.findById(userB.getId())).thenReturn(Optional.of(userB));
|
||||
when(notificationRepository.save(any())).thenAnswer(inv -> inv.getArgument(0));
|
||||
|
||||
notificationService.notifyMentions(List.of(userA.getId(), userB.getId()), comment);
|
||||
|
||||
ArgumentCaptor<Notification> captor = ArgumentCaptor.forClass(Notification.class);
|
||||
verify(notificationRepository, times(2)).save(captor.capture());
|
||||
|
||||
List<Notification> saved = captor.getAllValues();
|
||||
assertThat(saved).extracting(n -> n.getRecipient().getId())
|
||||
.containsExactlyInAnyOrder(userA.getId(), userB.getId());
|
||||
assertThat(saved).allMatch(n -> n.getType() == NotificationType.MENTION);
|
||||
}
|
||||
|
||||
@Test
|
||||
void notifyMentions_sendsEmailOnlyToUsersWithMentionNotificationsEnabled() {
|
||||
userA.setNotifyOnMention(true);
|
||||
userB.setNotifyOnMention(false);
|
||||
|
||||
DocumentComment comment = commentWithAuthor(UUID.randomUUID(), null, userC.getId());
|
||||
when(userRepository.findById(userA.getId())).thenReturn(Optional.of(userA));
|
||||
when(userRepository.findById(userB.getId())).thenReturn(Optional.of(userB));
|
||||
when(notificationRepository.save(any())).thenAnswer(inv -> inv.getArgument(0));
|
||||
|
||||
notificationService.notifyMentions(List.of(userA.getId(), userB.getId()), comment);
|
||||
|
||||
verify(mailSender, times(1)).send(any(SimpleMailMessage.class));
|
||||
}
|
||||
|
||||
// ─── markRead ─────────────────────────────────────────────────────────────
|
||||
|
||||
@Test
|
||||
void markRead_throwsForbidden_whenNotificationBelongsToDifferentUser() {
|
||||
Notification notification = Notification.builder()
|
||||
.id(UUID.randomUUID())
|
||||
.recipient(userA)
|
||||
.type(NotificationType.REPLY)
|
||||
.read(false)
|
||||
.build();
|
||||
|
||||
when(notificationRepository.findById(notification.getId())).thenReturn(Optional.of(notification));
|
||||
|
||||
assertThatThrownBy(() -> notificationService.markRead(notification.getId(), userB.getId()))
|
||||
.isInstanceOf(DomainException.class)
|
||||
.hasMessageContaining("different user");
|
||||
}
|
||||
|
||||
// ─── private helpers ──────────────────────────────────────────────────────
|
||||
|
||||
private DocumentComment commentWithAuthor(UUID id, UUID parentId, UUID authorId) {
|
||||
return DocumentComment.builder()
|
||||
.id(id)
|
||||
.documentId(UUID.randomUUID())
|
||||
.parentId(parentId)
|
||||
.authorId(authorId)
|
||||
.authorName("Author")
|
||||
.content("content")
|
||||
.build();
|
||||
}
|
||||
}
|
||||
⚠️ MAJOR — No
@RequirePermissionon notification controllerAll five notification endpoints have no
@RequirePermissionannotation. Resolving the current user viaauthentication.getName()is not the same as the AOPPermissionAspectgate — a disabled or low-privilege account that passes the Spring Security filter layer can still call these endpoints.The
NotificationControllerTesttests 401 for unauthenticated, but never tests that a user with no permissions is blocked (403). This gap means a regression here would not be caught by CI.Fix: Add
@RequirePermission(Permission.READ_ALL)at the controller class level. Update tests to cover the 403 case.