feature/expand-activity-log#13
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThis PR introduces comprehensive activity logging across the application. Controllers now capture authenticated user context via Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Controller
participant Service
participant ActivityLogService
participant Repository
participant Database
User->>Controller: POST /documents/upload (authenticated)
Controller->>Service: uploadDocument(username, file, ticketId)
Service->>Repository: save(document)
Repository->>Database: INSERT document
Repository-->>Service: saved document
Service->>ActivityLogService: createActivityLog(FILE_UPLOADED, description, userId, ticketId)
ActivityLogService->>Repository: save(activityLog)
Repository->>Database: INSERT activity_log
Repository-->>ActivityLogService: saved activity log
ActivityLogService-->>Service: success
Service-->>Controller: DocumentResponse
Controller-->>User: 201 Created
User->>Controller: PUT /tickets/{id}/status (authenticated)
Controller->>Service: updateStatus(username, ticketId, request)
Service->>Repository: findById(ticketId)
Repository->>Database: SELECT ticket
Repository-->>Service: ticket with old status
Service->>Repository: save(updated ticket)
Repository->>Database: UPDATE ticket
Service->>ActivityLogService: createActivityLog(TICKET_STATUS_CHANGED, description, userId, ticketId)
ActivityLogService->>Repository: save(activityLog)
Repository->>Database: INSERT activity_log
ActivityLogService-->>Service: success
Service-->>Controller: TicketResponse
Controller-->>User: 200 OK
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/main/java/org/group1/projectbackend/service/CommentService.java (2)
38-96:⚠️ Potential issue | 🟠 MajorInconsistent exception types:
CommentServicestill throws rawRuntimeExceptionon missing entities.This PR migrates
ActivityLogServiceto throwResourceNotFoundExceptionfor missingUser/SupportTicket/ActivityLog(and the controller layer is presumably mapping that to404), butCommentService.createComment(lines 40, 43),getCommentsBySupportTicketId(line 61),deleteComment(line 82), andupdateComment(line 96) still throw plainRuntimeException. That will surface as500 Internal Server Errorinstead of404 Not Foundfrom the comment endpoints.🛡️ Suggested diff
+import org.group1.projectbackend.exception.ResourceNotFoundException; @@ - .orElseThrow(() -> new RuntimeException("User not found with id: " + dto.getUserId())); + .orElseThrow(() -> new ResourceNotFoundException("User not found with id: " + dto.getUserId())); @@ - .orElseThrow(() -> new RuntimeException("Support ticket not found with id: " + dto.getSupportTicketId())); + .orElseThrow(() -> new ResourceNotFoundException("Support ticket not found with id: " + dto.getSupportTicketId())); @@ - .orElseThrow(() -> new RuntimeException("Support ticket not found with id: " + ticketId)); + .orElseThrow(() -> new ResourceNotFoundException("Support ticket not found with id: " + ticketId)); @@ - .orElseThrow(() -> new RuntimeException("Comment not found with id: " + id)); + .orElseThrow(() -> new ResourceNotFoundException("Comment not found with id: " + id)); @@ - .orElseThrow(() -> new RuntimeException("Comment not found with id: " + id)); + .orElseThrow(() -> new ResourceNotFoundException("Comment not found with id: " + id));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/group1/projectbackend/service/CommentService.java` around lines 38 - 96, The service currently throws raw RuntimeException when entities are missing; update createComment, getCommentsBySupportTicketId, deleteComment, and updateComment to throw ResourceNotFoundException instead of RuntimeException for user/supportTicket/comment lookups (the findById(...) orElseThrow(...) calls in those methods), preserving the same messages (e.g., "User not found with id: ...", "Support ticket not found with id: ...", "Comment not found with id: ..."); ensure you import ResourceNotFoundException and keep the logActivitySafely calls unchanged so controllers return 404s consistently.
80-106:⚠️ Potential issue | 🔴 CriticalAdd
@TransactionaltodeleteCommentandupdateCommentmethods.The DB constraints (
nullable = falseon bothuser_idandticket_idcolumns) and JPA mappings correctly enforce thatcomment.getUser()andcomment.getTicket()are non-null. However, both associations useFetchType.LAZYand the service methods lack@Transactionalannotations. WhenlogActivitySafelyattempts to accesscomment.getUser().getId()orcomment.getTicket().getId()after the repository operation, the Hibernate session will have closed, causing aLazyInitializationExceptionat runtime. BothdeleteComment(line 80) andupdateComment(line 94) must be annotated with@Transactionalto keep the session open for lazy initialization.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/group1/projectbackend/service/CommentService.java` around lines 80 - 106, The deleteComment and updateComment methods should be annotated with `@Transactional` so the Hibernate session remains open when logActivitySafely accesses lazy associations (comment.getUser().getId(), comment.getTicket().getId()); add `@Transactional` above both methods (or at the class level) and ensure the import for org.springframework.transaction.annotation.Transactional is present so repository operations and subsequent lazy access occur within a transactional context.src/main/java/org/group1/projectbackend/controller/web/DocumentViewController.java (1)
27-41:⚠️ Potential issue | 🟡 MinorPossible null
Principalmasked by the broad catch.If the
/documents/**route is reachable anonymously,principalwill be null, the NPE will bubble into theRuntimeExceptioncatch, and users will see "Dokumentet kunde inte tas bort." with no indication that it's an auth problem. Ensure the route requires authentication and/or add an explicit null check before callingprincipal.getName().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/group1/projectbackend/controller/web/DocumentViewController.java` around lines 27 - 41, The deleteDocument handler currently calls principal.getName() inside deleteDocument and masks null-principal NPEs in a broad RuntimeException catch; add an explicit null check for the Principal at the start of the deleteDocument method (or enforce authentication via an annotation like `@PreAuthorize/`@Secured on the controller/method) and handle the null case by setting an appropriate redirectAttributes.documentError (e.g., "Authentication required") and returning the redirect to /documents (or forward to login) before calling documentService.deleteDocument; ensure you still catch and log other RuntimeExceptions separately so auth-related nulls are not swallowed.
🧹 Nitpick comments (14)
src/test/java/org/group1/projectbackend/controller/CommentControllerTest.java (1)
27-27: Minor: prefer an import over the fully qualified name.The field type was changed to use the fully qualified
org.group1.projectbackend.service.CommentServicewhile the rest of the file uses imports. For consistency and readability, consider restoring the explicit import instead.♻️ Suggested diff
import org.group1.projectbackend.dto.comment.CommentDto; +import org.group1.projectbackend.service.CommentService; import org.junit.jupiter.api.BeforeEach; @@ `@MockitoBean` - private org.group1.projectbackend.service.CommentService commentService; + private CommentService commentService;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/group1/projectbackend/controller/CommentControllerTest.java` at line 27, Replace the fully-qualified field type with an import to match the file's style: add an import for CommentService and change the field declaration in CommentControllerTest from "private org.group1.projectbackend.service.CommentService commentService;" to use the simple type "CommentService" so the class uses consistent imports and improves readability.src/main/java/org/group1/projectbackend/service/ActivityLogService.java (1)
67-91: Inconsistent existence check between ticket-scoped and user-scoped log queries.
getActivityLogsBySupportTicketIdnow throwsResourceNotFoundExceptionwhen the ticket doesn't exist (good), butgetActivityLogsByUserIdsilently returns an empty list for a non-existentuserId. For API consistency and to help clients distinguish "user has no activity" from "user does not exist", consider adding the sameuserRepository.existsById(userId)pre-check.🛡️ Suggested diff
public List<ActivityLogDto> getActivityLogsByUserId(Long userId, String sortDirection) { + if (!userRepository.existsById(userId)) { + throw new ResourceNotFoundException("User not found with id: " + userId); + } + Sort sort = "desc".equalsIgnoreCase(sortDirection) ? Sort.by("createdAt").descending() : Sort.by("createdAt").ascending();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/group1/projectbackend/service/ActivityLogService.java` around lines 67 - 91, Add the same existence check used in getActivityLogsBySupportTicketId to getActivityLogsByUserId: call userRepository.existsById(userId) before building the Sort and querying activityLogRepository; if it returns false throw new ResourceNotFoundException("User not found with id: " + userId) so the API consistently signals a missing user rather than returning an empty list. Ensure you reference getActivityLogsByUserId, userRepository.existsById, and ResourceNotFoundException when making the change and keep the existing sort logic intact.src/main/java/org/group1/projectbackend/dto/activitylog/ActivityLogDto.java (1)
17-24: Positional-constructor coupling — caveat for callers.Adding
usernameandticketTitleand reordering fields changes the@AllArgsConstructorpositional signature. The mapper was updated to match, but any future caller using the all-args constructor (tests, serializers, JSON binding with constructor detection) must align with the new field order. Consider using@Builderat call sites to make these constructions resilient to further field additions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/group1/projectbackend/dto/activitylog/ActivityLogDto.java` around lines 17 - 24, The positional AllArgsConstructor for ActivityLogDto changed when fields (username, ticketTitle, field order) were modified; to avoid callers breaking on constructor argument order, add Lombok's `@Builder` to the ActivityLogDto class (in addition to keeping existing constructors/annotations like `@AllArgsConstructor/`@NoArgsConstructor) and update callers/tests/serializers that currently call the all-args constructor to use ActivityLogDto.builder().username(...).ticketTitle(...).activityType(...).description(...).createdAt(...).supportTicketId(...) so future field reordering/additions won't break positional construction.src/main/java/org/group1/projectbackend/service/DocumentService.java (1)
10-18: Inconsistent user-identity contract withinDocumentService.
deleteDocumentnow usesString username(server-side resolved fromPrincipal), whileuploadDocumentstill takes a client-suppliedLong uploadedByUserId. Per the retrieved learning, theuploadedByUserIdparameter is intentional until auth lands, but now thatPrincipalis being wired into controllers for activity logging, consider flippinguploadDocumentto also acceptString usernameso both methods resolve the acting user server-side. This would remove the last spoofable identity input on this interface and unify the activity-log provenance.Based on learnings:
DocumentService.uploadDocumentuses a client-supplieduploadedByUserIdintentionally until Spring Security auth is wired in — and this PR is part of that transition.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/group1/projectbackend/service/DocumentService.java` around lines 10 - 18, The interface has an inconsistent user identity contract: DocumentService.uploadDocument(Long uploadedByUserId, ...) accepts a client-supplied user ID while deleteDocument(String username, ...) uses server-resolved username; change uploadDocument to accept String username instead of Long uploadedByUserId so both methods resolve the acting user server-side. Update the DocumentService interface signature for uploadDocument, then update all implementing classes (e.g., DocumentServiceImpl.uploadDocument) and callers (controllers that currently pass uploadedByUserId) to pass Principal.getName() or the server-resolved username; adjust unit/integration tests and any DTO mapping that referenced uploadedByUserId accordingly. Ensure method name and parameter order remain otherwise unchanged (DocumentService.uploadDocument(String username, MultipartFile file, Long ticketId) or matching existing parameter order) so callers compile. Make sure to remove or deprecate the old uploadedByUserId usage to prevent duplicate identity inputs.src/test/java/org/group1/projectbackend/service/CommentServiceTest.java (1)
49-49: Nit: redundant fully-qualified class name.
CommentServicelives in the same package as this test (org.group1.projectbackend.service), so the FQCN is unnecessary and just adds visual noise. A plainprivate CommentService commentService;is enough.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/group1/projectbackend/service/CommentServiceTest.java` at line 49, The field declaration uses a redundant fully-qualified name; change the field declaration "private org.group1.projectbackend.service.CommentService commentService;" to use the simple type name "CommentService" (i.e., "private CommentService commentService;") to match the test's package and remove visual noise while keeping the same symbol name commentService.src/main/java/org/group1/projectbackend/service/impl/SupportTicketServiceImpl.java (1)
102-113: ReplaceSystem.err.printlnwith a proper logger, and consider extracting the duplicated helper.Two related concerns in
logActivitySafely:
System.err.printlnbypasses the application's logging pipeline (no levels, no MDC, no timestamps/thread info, no structured output). Use SLF4J and log atwarnwith the exception so the stack trace is preserved — otherwise a silently-swallowed bug inActivityLogServicewill be very hard to diagnose in production.- This exact helper is duplicated in
CommentService.logActivitySafely(per the provided snippet fromsrc/main/java/org/group1/projectbackend/service/CommentService.java:112-123). As more services adopt activity logging, the pattern will keep multiplying. Consider extracting a smallSafeActivityLoggercomponent that wrapsActivityLogServiceand exposes a singlelogSafely(...)method.♻️ Minimal diff: switch to SLF4J
+import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + `@Service` public class SupportTicketServiceImpl implements SupportTicketService { + private static final Logger log = LoggerFactory.getLogger(SupportTicketServiceImpl.class); + private final SupportTicketRepository supportTicketRepository;private void logActivitySafely(ActivityType activityType, String description, Long userId, Long ticketId) { try { activityLogService.createActivityLog(new CreateActivityLogDto( activityType, description, userId, ticketId )); } catch (RuntimeException ex) { - System.err.println("Failed to create activity log: " + ex.getMessage()); + log.warn("Failed to create activity log (type={}, userId={}, ticketId={})", + activityType, userId, ticketId, ex); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/group1/projectbackend/service/impl/SupportTicketServiceImpl.java` around lines 102 - 113, Replace the System.err.println in SupportTicketServiceImpl.logActivitySafely with an SLF4J logger call and preserve the exception (e.g., logger.warn("Failed to create activity log for ticketId={}, userId={}", ticketId, userId, ex)); then extract the duplicated logic from SupportTicketServiceImpl.logActivitySafely and CommentService.logActivitySafely into a single reusable component/class (e.g., SafeActivityLogger) that wraps ActivityLogService and exposes a logSafely(ActivityType, String, Long userId, Long ticketId) method; update both services to inject and use SafeActivityLogger instead of their private helpers so logging uses SLF4J and the helper is not duplicated.src/test/java/org/group1/projectbackend/controller/DocumentControllerTest.java (1)
167-173: Consider verifying the forwarded principal name.The stub uses
any()for the username, so a regression where the controller sendsnullor a hard-coded value would not be caught. Considerverify(documentService).deleteDocument(eq("user"), eq(100L));(default@WithMockUserusername isuser) to pin the contract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/group1/projectbackend/controller/DocumentControllerTest.java` around lines 167 - 173, Update the test shouldDeleteDocument to assert the controller forwards the authenticated principal to the service: replace the loose doNothing/mock verification that uses any() for the username with a strict verify call on documentService.deleteDocument(eq("user"), eq(100L)) (the default `@WithMockUser` username is "user") so the test fails if the controller passes null or a hard-coded value; keep the existing mockMvc.perform(delete("/api/documents/100").with(csrf())) and status assertion but add the verify(documentService).deleteDocument(...) after the request to enforce the principal contract.src/main/java/org/group1/projectbackend/controller/web/ActivityLogViewController.java (2)
67-70: Ticket-id filter produces false positives.
String.valueOf(id).contains(expected)makes e.g.ticket=2match ids12,20,200. If the intent is "match by ID", prefer an exact parse (Long.parseLong(expected)) and fall back to the title substring match only when parsing fails.♻️ Proposed tweak
- private boolean matchesTicketId(ActivityLogDto activityLog, String expected) { - return activityLog.getSupportTicketId() != null - && String.valueOf(activityLog.getSupportTicketId()).contains(expected); - } + private boolean matchesTicketId(ActivityLogDto activityLog, String expected) { + if (activityLog.getSupportTicketId() == null) { + return false; + } + try { + return activityLog.getSupportTicketId().equals(Long.parseLong(expected)); + } catch (NumberFormatException ex) { + return false; + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/group1/projectbackend/controller/web/ActivityLogViewController.java` around lines 67 - 70, The matchesTicketId method currently uses String.valueOf(activityLog.getSupportTicketId()).contains(expected) which causes false positives; change it to attempt parsing the expected string to a Long (e.g., using Long.parseLong(expected)) and compare for equality with activityLog.getSupportTicketId() (via ActivityLogDto.getSupportTicketId()); if parsing throws/ fails, fall back to the previous substring behavior but applied to the ticket title (e.g., ActivityLogDto.getSupportTicketTitle().contains(expected)) or to a safe string conversion only as a fallback so numeric ID matching is exact while non-numeric queries still allow title substring matching.
34-43: Move filtering/sorting to the repository layer.Fetching all activity logs into memory then sorting and filtering in a stream won't scale and is vulnerable to
LazyInitializationExceptionwhen the mapper readsuser.username/supportTicket.titleoutside a transaction. Consider a repository query with JPASpecification/derived queries and a@QueryusingJOIN FETCH(or@EntityGraph) onuserandsupportTicket, plus passingSort.by("createdAt").descending()and pagination.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/group1/projectbackend/controller/web/ActivityLogViewController.java` around lines 34 - 43, The controller currently loads all logs via activityLogService.getAllActivityLogs() and applies stream-based sorting/filtering which causes poor scalability and LazyInitializationException; move this logic into the data layer by adding repository-level query methods (e.g., in ActivityLogRepository) that accept a JPA Specification or derived parameters and return results with Sort/Pageable (use Sort.by("createdAt").descending() and Pageable for pagination). Ensure the repository query uses JOIN FETCH or an `@EntityGraph` to eagerly fetch user and supportTicket to avoid lazy-load outside a transaction, add a service method like findActivityLogs(Specification<ActivityLog>, Pageable) that the ActivityLogViewController calls with built Specifications for type, username, ticket and date, and return the sorted/paged list instead of filtering in the controller.src/main/resources/templates/tickets/list.html (1)
39-42: Consider CSS truncation instead of substring.
#strings.substringtruncates by UTF-16 code units, which can split surrogate pairs/emoji and always shows "..." even when not needed. A pure-CSS approach (max-width+white-space: nowrap; overflow: hidden; text-overflow: ellipsis;) is more robust and keeps the full text selectable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/templates/tickets/list.html` around lines 39 - 42, Replace the server-side substring truncation in the table cell (the expression using `#strings.substring` and the conditional) with a plain full title rendering (keep th:title="${ticket.title()}" and set th:text="${ticket.title()}"), and add a CSS class (e.g. class="truncate") to that <td> so truncation is handled in CSS; then implement the CSS rules for .truncate: max-width (or width), white-space: nowrap, overflow: hidden, and text-overflow: ellipsis so the displayed title is visually truncated without cutting surrogate pairs and the full text remains selectable via the title attribute.src/main/resources/templates/activitylogs/list.html (2)
29-34: Nit: placeholder text inside<option>is redundant.
th:text="${activityType.name()}"already sets the content at render time; theTICKET_CREATEDliteral is only visible at design-time and can mislead static-HTML previews. Minor, keep if you rely on preview tooling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/templates/activitylogs/list.html` around lines 29 - 34, Remove the static placeholder text "TICKET_CREATED" inside the <option> element since th:text="${activityType.name()}" will render the option label at runtime; update the template block using th:each, th:value, th:text and th:selected (the option with th:each="activityType : ${activityTypes}" and th:value/th:text/th:selected attributes) to have no inner literal content so design-time previews don't show misleading static text.
74-84: Filter-summary separator logic is brittle.The hand-rolled comma separators between filter tokens replicate the conjunction logic four times and will be tedious to maintain if new filters are added. Consider joining active filter labels via a controller-supplied list or a Thymeleaf utility like
#strings.listJoin(...).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/templates/activitylogs/list.html` around lines 74 - 84, The template's comma/separator logic for showing active filters (using selectedType, selectedUsername, selectedTicket, selectedDate in list.html) is brittle and duplicated; instead prepare an ordered list of active filter labels in the controller (e.g., create List<String> activeFilters from the same selection logic) or build the list in Thymeleaf and join with a utility (e.g., `#strings.listJoin`(activeFilters, ', ')), then render a single span that prints the joined string (remove the repeated span/separator blocks). Ensure the controller or template produces labels like "Typ: X", "Användare: Y", "Ticket: Z", "Datum: D" and pass them as the variable used by the join.src/test/java/org/group1/projectbackend/service/DocumentServiceTest.java (1)
123-128: Consider adding a negative-path test for activity-log failures.The tests cover the happy path for
createActivityLoginvocations. SinceDocumentServiceImpl.logActivitySafelyis designed to swallowRuntimeExceptionthrown byActivityLogService, it would be valuable to add one test stubbingactivityLogService.createActivityLog(...)to throw, and asserting thatuploadDocument/deleteDocumentstill complete successfully (document saved/deleted, storage object handled). This locks in the intended resilience guarantee.Also applies to: 190-195
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/group1/projectbackend/service/DocumentServiceTest.java` around lines 123 - 128, Add negative-path unit tests that verify DocumentServiceImpl.logActivitySafely swallows runtime exceptions from ActivityLogService: stub activityLogService.createActivityLog(...) to throw a RuntimeException and then call uploadDocument (and a separate test for deleteDocument) and assert the primary operation still completes successfully (document persisted or deleted) and storage interactions (e.g., storageClient.putObject or storageClient.deleteObject) are invoked as expected; also verify no exception escapes and that activity logging was attempted (verify createActivityLog was called) even though it threw.src/main/java/org/group1/projectbackend/controller/web/TicketViewController.java (1)
61-61: Consider replacing"desc"/"asc"magic strings with an enum or constants.
activityLogService.getActivityLogsBySupportTicketId(id, "desc")relies on a free-form string for sort direction. A dedicatedSortDirectionenum (or reuse of Spring'sSort.Direction) would be safer and self-documenting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/group1/projectbackend/controller/web/TicketViewController.java` at line 61, Replace the "desc"/"asc" magic string usage in TicketViewController's call to activityLogService.getActivityLogsBySupportTicketId(id, "desc") by switching to a typed enum or Spring's Sort.Direction: update the activityLogService method signature (e.g., getActivityLogsBySupportTicketId(Long id, Sort.Direction direction) or use your own SortDirection enum) and change the controller call to pass Sort.Direction.DESC (or SortDirection.DESC) instead of the literal "desc"; update any downstream usages and converters/parsers accordingly so the service and controller use the enum type rather than free-form strings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/org/group1/projectbackend/controller/ActivityLogController.java`:
- Line 47: The controller changed the default sortDirection from "asc" to "desc"
for the activity-log endpoint, which is a behavioral API change; either align
the other user-pivot endpoint in ActivityLogController so both
`@RequestParam`(defaultValue = "desc") String sortDirection use the same
newest-first default, or explicitly document the deliberate difference in the
API docs/changelog and tests; update the other occurrence of the sortDirection
default (currently "asc") in ActivityLogController to "desc" if you choose
alignment, and add a changelog/API docs note calling out the changed default
behavior so clients that omit the parameter are aware.
In
`@src/main/java/org/group1/projectbackend/controller/web/ActivityLogViewController.java`:
- Line 35: The current sort lambda in ActivityLogViewController (.sorted((left,
right) -> right.getCreatedAt().compareTo(left.getCreatedAt()))) can NPE when
createdAt is null; replace it with a null-safe comparator such as
Comparator.comparing(ActivityLogDto::getCreatedAt,
Comparator.nullsLast(Comparator.naturalOrder())).reversed() or ensure createdAt
is non-null at persistence (e.g., add `@CreationTimestamp/`@Column(nullable =
false)) and then use the safer comparator to prevent null pointer exceptions
during sorting.
In `@src/main/java/org/group1/projectbackend/service/CommentService.java`:
- Around line 112-123: Replace the System.err usage in
CommentService.logActivitySafely with an SLF4J logger: add a private static
final Logger (LoggerFactory.getLogger(CommentService.class)) and change the
catch to logger.error or logger.warn with a descriptive message and pass the
exception (e.g., logger.error("Failed to create activity log for userId={}
ticketId={}", userId, ticketId, ex)); apply the same fix to
DocumentServiceImpl.logActivitySafely and consider extracting a shared utility
method to avoid duplication.
In
`@src/main/java/org/group1/projectbackend/service/impl/DocumentServiceImpl.java`:
- Around line 196-207: The catch around activityLogService.createActivityLog in
logActivitySafely swallows RuntimeException (which still marks the outer
`@Transactional` uploadDocument as rollback-only) and uses System.err; fix by
isolating the activity write or deferring it and replace the stderr call with
SLF4J: either change ActivityLogService.createActivityLog to run in its own
transaction (annotate with `@Transactional`(propagation = REQUIRES_NEW) on the
method) so failures don't mark the caller txn, or schedule the
CreateActivityLogDto via
TransactionSynchronizationManager.registerSynchronization(...).afterCommit(...)
from uploadDocument so logging runs post-commit; additionally replace
System.err.println with a logger (use a private static final Logger and call
logger.error(...) including exception) in logActivitySafely and any related code
paths.
In `@src/main/resources/templates/fragments/layout.html`:
- Line 14: Replace the English anchor text "Activity Log" in the navigation link
(the <a> element with th:href="@{/activitylogs/view}") with the Swedish label
"Aktivitetslogg" so it matches the other Swedish nav items in layout.html;
update only the anchor's inner text to "Aktivitetslogg" (or the corresponding
i18n key if the project uses message bundles) to maintain consistency.
In `@src/main/resources/templates/tickets/list.html`:
- Line 47: Replace the invalid nested <a> inside <button> by using a single
interactive element: either turn the outer element into the anchor (keep
th:href="@{/tickets/{id}(id=${ticket.id()})}" and the link text "Visa", adding a
CSS class like "btn" for button styling) or keep a <button type="button"> and
wire navigation via th:onclick (using the same URL expression with ticket.id()).
Locate the element with th:href and ticket.id() in the template and remove the
nested structure so only one interactive element (anchor or button) remains.
---
Outside diff comments:
In
`@src/main/java/org/group1/projectbackend/controller/web/DocumentViewController.java`:
- Around line 27-41: The deleteDocument handler currently calls
principal.getName() inside deleteDocument and masks null-principal NPEs in a
broad RuntimeException catch; add an explicit null check for the Principal at
the start of the deleteDocument method (or enforce authentication via an
annotation like `@PreAuthorize/`@Secured on the controller/method) and handle the
null case by setting an appropriate redirectAttributes.documentError (e.g.,
"Authentication required") and returning the redirect to /documents (or forward
to login) before calling documentService.deleteDocument; ensure you still catch
and log other RuntimeExceptions separately so auth-related nulls are not
swallowed.
In `@src/main/java/org/group1/projectbackend/service/CommentService.java`:
- Around line 38-96: The service currently throws raw RuntimeException when
entities are missing; update createComment, getCommentsBySupportTicketId,
deleteComment, and updateComment to throw ResourceNotFoundException instead of
RuntimeException for user/supportTicket/comment lookups (the findById(...)
orElseThrow(...) calls in those methods), preserving the same messages (e.g.,
"User not found with id: ...", "Support ticket not found with id: ...", "Comment
not found with id: ..."); ensure you import ResourceNotFoundException and keep
the logActivitySafely calls unchanged so controllers return 404s consistently.
- Around line 80-106: The deleteComment and updateComment methods should be
annotated with `@Transactional` so the Hibernate session remains open when
logActivitySafely accesses lazy associations (comment.getUser().getId(),
comment.getTicket().getId()); add `@Transactional` above both methods (or at the
class level) and ensure the import for
org.springframework.transaction.annotation.Transactional is present so
repository operations and subsequent lazy access occur within a transactional
context.
---
Nitpick comments:
In
`@src/main/java/org/group1/projectbackend/controller/web/ActivityLogViewController.java`:
- Around line 67-70: The matchesTicketId method currently uses
String.valueOf(activityLog.getSupportTicketId()).contains(expected) which causes
false positives; change it to attempt parsing the expected string to a Long
(e.g., using Long.parseLong(expected)) and compare for equality with
activityLog.getSupportTicketId() (via ActivityLogDto.getSupportTicketId()); if
parsing throws/ fails, fall back to the previous substring behavior but applied
to the ticket title (e.g.,
ActivityLogDto.getSupportTicketTitle().contains(expected)) or to a safe string
conversion only as a fallback so numeric ID matching is exact while non-numeric
queries still allow title substring matching.
- Around line 34-43: The controller currently loads all logs via
activityLogService.getAllActivityLogs() and applies stream-based
sorting/filtering which causes poor scalability and LazyInitializationException;
move this logic into the data layer by adding repository-level query methods
(e.g., in ActivityLogRepository) that accept a JPA Specification or derived
parameters and return results with Sort/Pageable (use
Sort.by("createdAt").descending() and Pageable for pagination). Ensure the
repository query uses JOIN FETCH or an `@EntityGraph` to eagerly fetch user and
supportTicket to avoid lazy-load outside a transaction, add a service method
like findActivityLogs(Specification<ActivityLog>, Pageable) that the
ActivityLogViewController calls with built Specifications for type, username,
ticket and date, and return the sorted/paged list instead of filtering in the
controller.
In
`@src/main/java/org/group1/projectbackend/controller/web/TicketViewController.java`:
- Line 61: Replace the "desc"/"asc" magic string usage in TicketViewController's
call to activityLogService.getActivityLogsBySupportTicketId(id, "desc") by
switching to a typed enum or Spring's Sort.Direction: update the
activityLogService method signature (e.g., getActivityLogsBySupportTicketId(Long
id, Sort.Direction direction) or use your own SortDirection enum) and change the
controller call to pass Sort.Direction.DESC (or SortDirection.DESC) instead of
the literal "desc"; update any downstream usages and converters/parsers
accordingly so the service and controller use the enum type rather than
free-form strings.
In `@src/main/java/org/group1/projectbackend/dto/activitylog/ActivityLogDto.java`:
- Around line 17-24: The positional AllArgsConstructor for ActivityLogDto
changed when fields (username, ticketTitle, field order) were modified; to avoid
callers breaking on constructor argument order, add Lombok's `@Builder` to the
ActivityLogDto class (in addition to keeping existing constructors/annotations
like `@AllArgsConstructor/`@NoArgsConstructor) and update
callers/tests/serializers that currently call the all-args constructor to use
ActivityLogDto.builder().username(...).ticketTitle(...).activityType(...).description(...).createdAt(...).supportTicketId(...)
so future field reordering/additions won't break positional construction.
In `@src/main/java/org/group1/projectbackend/service/ActivityLogService.java`:
- Around line 67-91: Add the same existence check used in
getActivityLogsBySupportTicketId to getActivityLogsByUserId: call
userRepository.existsById(userId) before building the Sort and querying
activityLogRepository; if it returns false throw new
ResourceNotFoundException("User not found with id: " + userId) so the API
consistently signals a missing user rather than returning an empty list. Ensure
you reference getActivityLogsByUserId, userRepository.existsById, and
ResourceNotFoundException when making the change and keep the existing sort
logic intact.
In `@src/main/java/org/group1/projectbackend/service/DocumentService.java`:
- Around line 10-18: The interface has an inconsistent user identity contract:
DocumentService.uploadDocument(Long uploadedByUserId, ...) accepts a
client-supplied user ID while deleteDocument(String username, ...) uses
server-resolved username; change uploadDocument to accept String username
instead of Long uploadedByUserId so both methods resolve the acting user
server-side. Update the DocumentService interface signature for uploadDocument,
then update all implementing classes (e.g., DocumentServiceImpl.uploadDocument)
and callers (controllers that currently pass uploadedByUserId) to pass
Principal.getName() or the server-resolved username; adjust unit/integration
tests and any DTO mapping that referenced uploadedByUserId accordingly. Ensure
method name and parameter order remain otherwise unchanged
(DocumentService.uploadDocument(String username, MultipartFile file, Long
ticketId) or matching existing parameter order) so callers compile. Make sure to
remove or deprecate the old uploadedByUserId usage to prevent duplicate identity
inputs.
In
`@src/main/java/org/group1/projectbackend/service/impl/SupportTicketServiceImpl.java`:
- Around line 102-113: Replace the System.err.println in
SupportTicketServiceImpl.logActivitySafely with an SLF4J logger call and
preserve the exception (e.g., logger.warn("Failed to create activity log for
ticketId={}, userId={}", ticketId, userId, ex)); then extract the duplicated
logic from SupportTicketServiceImpl.logActivitySafely and
CommentService.logActivitySafely into a single reusable component/class (e.g.,
SafeActivityLogger) that wraps ActivityLogService and exposes a
logSafely(ActivityType, String, Long userId, Long ticketId) method; update both
services to inject and use SafeActivityLogger instead of their private helpers
so logging uses SLF4J and the helper is not duplicated.
In `@src/main/resources/templates/activitylogs/list.html`:
- Around line 29-34: Remove the static placeholder text "TICKET_CREATED" inside
the <option> element since th:text="${activityType.name()}" will render the
option label at runtime; update the template block using th:each, th:value,
th:text and th:selected (the option with th:each="activityType :
${activityTypes}" and th:value/th:text/th:selected attributes) to have no inner
literal content so design-time previews don't show misleading static text.
- Around line 74-84: The template's comma/separator logic for showing active
filters (using selectedType, selectedUsername, selectedTicket, selectedDate in
list.html) is brittle and duplicated; instead prepare an ordered list of active
filter labels in the controller (e.g., create List<String> activeFilters from
the same selection logic) or build the list in Thymeleaf and join with a utility
(e.g., `#strings.listJoin`(activeFilters, ', ')), then render a single span that
prints the joined string (remove the repeated span/separator blocks). Ensure the
controller or template produces labels like "Typ: X", "Användare: Y", "Ticket:
Z", "Datum: D" and pass them as the variable used by the join.
In `@src/main/resources/templates/tickets/list.html`:
- Around line 39-42: Replace the server-side substring truncation in the table
cell (the expression using `#strings.substring` and the conditional) with a plain
full title rendering (keep th:title="${ticket.title()}" and set
th:text="${ticket.title()}"), and add a CSS class (e.g. class="truncate") to
that <td> so truncation is handled in CSS; then implement the CSS rules for
.truncate: max-width (or width), white-space: nowrap, overflow: hidden, and
text-overflow: ellipsis so the displayed title is visually truncated without
cutting surrogate pairs and the full text remains selectable via the title
attribute.
In
`@src/test/java/org/group1/projectbackend/controller/CommentControllerTest.java`:
- Line 27: Replace the fully-qualified field type with an import to match the
file's style: add an import for CommentService and change the field declaration
in CommentControllerTest from "private
org.group1.projectbackend.service.CommentService commentService;" to use the
simple type "CommentService" so the class uses consistent imports and improves
readability.
In
`@src/test/java/org/group1/projectbackend/controller/DocumentControllerTest.java`:
- Around line 167-173: Update the test shouldDeleteDocument to assert the
controller forwards the authenticated principal to the service: replace the
loose doNothing/mock verification that uses any() for the username with a strict
verify call on documentService.deleteDocument(eq("user"), eq(100L)) (the default
`@WithMockUser` username is "user") so the test fails if the controller passes
null or a hard-coded value; keep the existing
mockMvc.perform(delete("/api/documents/100").with(csrf())) and status assertion
but add the verify(documentService).deleteDocument(...) after the request to
enforce the principal contract.
In `@src/test/java/org/group1/projectbackend/service/CommentServiceTest.java`:
- Line 49: The field declaration uses a redundant fully-qualified name; change
the field declaration "private org.group1.projectbackend.service.CommentService
commentService;" to use the simple type name "CommentService" (i.e., "private
CommentService commentService;") to match the test's package and remove visual
noise while keeping the same symbol name commentService.
In `@src/test/java/org/group1/projectbackend/service/DocumentServiceTest.java`:
- Around line 123-128: Add negative-path unit tests that verify
DocumentServiceImpl.logActivitySafely swallows runtime exceptions from
ActivityLogService: stub activityLogService.createActivityLog(...) to throw a
RuntimeException and then call uploadDocument (and a separate test for
deleteDocument) and assert the primary operation still completes successfully
(document persisted or deleted) and storage interactions (e.g.,
storageClient.putObject or storageClient.deleteObject) are invoked as expected;
also verify no exception escapes and that activity logging was attempted (verify
createActivityLog was called) even though it threw.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1303e569-4096-4f40-ace8-e237cde69b11
📒 Files selected for processing (29)
src/main/java/org/group1/projectbackend/controller/ActivityLogController.javasrc/main/java/org/group1/projectbackend/controller/DocumentController.javasrc/main/java/org/group1/projectbackend/controller/SupportTicketController.javasrc/main/java/org/group1/projectbackend/controller/web/ActivityLogViewController.javasrc/main/java/org/group1/projectbackend/controller/web/DocumentViewController.javasrc/main/java/org/group1/projectbackend/controller/web/TicketViewController.javasrc/main/java/org/group1/projectbackend/dto/activitylog/ActivityLogDto.javasrc/main/java/org/group1/projectbackend/mapper/ActivityLogMapper.javasrc/main/java/org/group1/projectbackend/service/ActivityLogService.javasrc/main/java/org/group1/projectbackend/service/CommentService.javasrc/main/java/org/group1/projectbackend/service/DocumentService.javasrc/main/java/org/group1/projectbackend/service/SupportTicketService.javasrc/main/java/org/group1/projectbackend/service/impl/DocumentServiceImpl.javasrc/main/java/org/group1/projectbackend/service/impl/SupportTicketServiceImpl.javasrc/main/resources/static/css/app.csssrc/main/resources/templates/activitylogs/list.htmlsrc/main/resources/templates/documents/list.htmlsrc/main/resources/templates/fragments/layout.htmlsrc/main/resources/templates/tickets/detail.htmlsrc/main/resources/templates/tickets/list.htmlsrc/test/java/org/group1/projectbackend/controller/ActivityLogControllerTest.javasrc/test/java/org/group1/projectbackend/controller/CommentControllerTest.javasrc/test/java/org/group1/projectbackend/controller/DocumentControllerTest.javasrc/test/java/org/group1/projectbackend/controller/SupportTicketControllerTest.javasrc/test/java/org/group1/projectbackend/mapper/ActivityLogMapperTest.javasrc/test/java/org/group1/projectbackend/service/ActivityLogServiceTest.javasrc/test/java/org/group1/projectbackend/service/CommentServiceTest.javasrc/test/java/org/group1/projectbackend/service/DocumentServiceTest.javasrc/test/java/org/group1/projectbackend/service/SupportTicketServiceTest.java
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
1 participant
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 by CodeRabbit
Release Notes